Change 333320 had a related patch set uploaded (by Bmansurov):
WIP: RESTBase

https://gerrit.wikimedia.org/r/333320

@ovasileva, we have some logic in the codebase that hides the thumbnail of a popup if it doesn't meet some criteria. For example, if an image width is bigger than the image height (i.e. a landscape image), then this width must at least be 300px wide. For landscape images, the height must be at least 250px tall. Here is the related code.

RESTBase only provides thumbnails with the maximum dimension of 320px. Considering hi-DPI monitors, this value will even be smaller when normalized. What all this means is that given the status quo if we switch to RESTBase end-point, no thumbnail will show. Is that acceptable? Can we live with that until there is an official thumb API that will be able to let clients specify dimensions? Any other suggestions?

Please review the approach. I'll add tests after the initial review.

@phuedx I thought 320px was wide enough for the page previews? Seems like @bmansurov is thinking that it isn't. What size do you guys need to make it useful?

@Jhernandez are language variants still an issue for this?

In T123445#2963831, @Fjalapeno wrote:

@Jhernandez are language variants still an issue for this?

They shouldn't be. The RESTBase API is returning the page language code and direction.

In T123445#2963826, @Fjalapeno wrote:

@phuedx I thought 320px was wide enough for the page previews? Seems like @bmansurov is thinking that it isn't. What size do you guys need to make it useful?

This was mibad. I forgot that the existing implementation requests an image appropriate for the pixel density of the screen. With that in mind, how do the Android and iOS apps handle this? They're both using the RESTBase API for phones with a (likely) minimum screen width of 640px+, right?

@bmansurov - do you know what percentage of thumbnails this would affect?

@ovasileva I don't have an exact number, but my wild guess is that this will affect the majority of thumbnails.

@phuedx I think iOS is ok here, but I think the discussion that led to it being kept at 320 was due to both Android and external clients.

Let me file a ticket explicitly to discus this issue.

@phuedx Actually, we have a canonical image update that is getting deployed next week:

T152163

Will this be good enough for your use case, or would you still look to discus using the thumb size specifically?

In T123445#2965505, @Fjalapeno wrote:

@phuedx Actually, we have a canonical image update that is getting deployed next week:

T152163

@bmansurov ^

Knowing that the canonical image update is being deployed next week, we have a handful of options available:

1. Don't display images on Page Previews delivered by RESTBase (by choice rather than by circumstance).
2. Drop the size restriction and likely display stretched thumbnails.
3. Determine the URL of the thumbnail that satisfies the size constraint from the URL of the original thumbnail.

@phuedx is there a library/function that handles figuring out the URL for the thumbnail you want? I thought I heard someone mention that before.

@ovasileva , thanks for the ping. Looks like RESTBase hasn't implemented the new functionality introduced in PageImages yet. I've pinged Bernd to know more. In the mean-time I'll be working on other parts of the patch and waiting on review for the existing code.

FYI: this is getting deployed today:
T152163

@phuedx based on our conversation, I filed this ticket:
T156387

The good news is that there is movement in the thumbnail API, it seems we already have one… kinda… see comments:
T66214

The format thus far is like:

https://commons.wikimedia.org/w/thumb.php?f=Camera2_mgx.svg&w=100&lang=fr

@phuedx will this meet your MVP requirements? Do you have additional enhancements that I can file against this ticket?

As far as timing… unfortunately this feature hasn't had a real champion or owner. But now that Web is interested for a specific feature, we basically have all of reading on board. So I can work with Gabriel to get this work moved along and prioritized.

However, I don't see this being complete in time for this ticket, so I still recommend using the canonical URL that is returned and rewriting it to your required size.

Is this an acceptable solution for now?

cc @ovasileva @Jdlrobson @bmansurov

In T123445#2974353, @Fjalapeno wrote:

Is this an acceptable solution for now?

Once the RESTBase API has been updated to include the URL of the original image in it – also tracked in T152163: Update RESTBase page summary endpoint to return canonical "image" info for each "thumbnail" per T152163#2970155 – then we can generate the URL for the 320px, 480px, or 640px thumbnail as required.

Change 335127 had a related patch set uploaded (by Bmansurov):
Add support for RESTBase endpoint consumption

https://gerrit.wikimedia.org/r/335127

Change 335127 abandoned by Bmansurov:
Add support for RESTBase endpoint consumption

Reason:
wrong branch

https://gerrit.wikimedia.org/r/335127

@ovasileva since T152163#2982949 is done, we should probably create a ticket to make Page Previews images work with the new RESTBase format. It seems outside the scope of this task.

I have added some comments to https://gerrit.wikimedia.org/r/#/c/333320/8

We chatted in the engineering sync and @bmansurov mentioned he's going to be doing some more changes so back to more work

Change 333320 merged by jenkins-bot:
Add support for RESTBase endpoint consumption

https://gerrit.wikimedia.org/r/333320

Change 336392 had a related patch set uploaded (by Phuedx):
Hygiene: Use factory functions instead of classes

https://gerrit.wikimedia.org/r/336392

Change 336606 had a related patch set uploaded (by Phuedx):
Hygiene: Split and organize the gateways

https://gerrit.wikimedia.org/r/336606

^ @Jhernandez's follow-up changes.

The last patch https://gerrit.wikimedia.org/r/#/c/336606 has a +1 from me

Change 336606 merged by jenkins-bot:
Hygiene: Split and organize the gateways

https://gerrit.wikimedia.org/r/336606

Could we take the time to set up RESTBase on the staging server?

^ The last AC was invalid as using the MediaWiki API is the default strategy.

@ovasileva let me know if you'd like me to bring back T123445#3011042 on the staging server for testing.

For the record, the team decided not to setup RESTBase on the staging server at this time.

Could you expand on why you see a need to normalize the title in the client? Unless I'm missing something, the href targets provided in both PHP parser & Parsoid HTML should already be normalized.

@bmansurov - yes, could you? I just want to test the changes quickly to see if it was due to browser extensions. No need for setting up RESTBase

@Jhernandez the normalize part was added by you in T123445#2947254, would you like to answer T123445#3014269?

@ovasileva done.

The title parameter is normalised by the client before making the API call so as to avoid 304s.

This is already done by mw.popups.processLinks.

In T123445#3014269, @GWicke wrote:

Could you expand on why you see a need to normalize the title in the client? Unless I'm missing something, the href targets provided in both PHP parser & Parsoid HTML should already be normalized.

@GWicke: It turns out that we do use the normalized title from the href but I couldn't remember whether that was the case when we were fleshing out this task. This was more of a note to ourselves that we should be using normalized titles rather than, say, the (non-normalized) title attribute of the link.

All works as expected. Signed off from my end. @phuedx, @bmansurov - ready to resolve?

Yes.

In T123445#3016393, @phuedx wrote:

In T123445#3014269, @GWicke wrote:

Could you expand on why you see a need to normalize the title in the client? Unless I'm missing something, the href targets provided in both PHP parser & Parsoid HTML should already be normalized.

@GWicke: It turns out that we do use the normalized title from the href but I couldn't remember whether that was the case when we were fleshing out this task. This was more of a note to ourselves that we should be using normalized titles rather than, say, the (non-normalized) title attribute of the link.

Makes sense. Thanks for the explanation!