Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16610002:
High DPI support for themes: Raw images for NTP attribution and background (Closed)
|
Created:
7 years, 6 months ago by sschmitz Modified:
7 years, 6 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, estade+watch_chromium.org, arv+watch_chromium.org, pedrosimonetti+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHigh DPI support for themes: Raw images for NTP attribution and background
Adding support for the NTP background and attribution theme
images. They are stored in the so-called raw images in
BrowserThemePack. Added computation of images for missing scales from
availables scales. Changed *.css and *.js files so that the URLs
contain the requisite 1x or 2x indication. (This depends on Issue 248189)
Reviewers:
pkotwicz: overall
xiyuan: for *.css and *.js
BUG=133934
R=pkotwicz@chromium.org
TEST=manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=205862
Patch Set 1 #
Total comments: 10
Patch Set 2 : review #
Total comments: 10
Patch Set 3 : minor #Patch Set 4 : update css #
Messages
Total messages: 24 (0 generated)
- I think that it is useful to write a CL introducing IDR_THEME_DETACHED_BOOKMARK_BAR_NTP_BACKGROUND before landing this one. This guarantees that this CL will not regress performance when the NTP has a detached bookmark bar. - As a heads up, I do not think your CL affects the new tab page when running with --instant-extended-api (I do not mind if that gets fixed as part of a separate CL) I will continue reviewing the changes to this CL with the above disclaimers https://codereview.chromium.org/16610002/diff/1/chrome/browser/resources/new_... File chrome/browser/resources/new_incognito_tab_theme.css (right): https://codereview.chromium.org/16610002/diff/1/chrome/browser/resources/new_... chrome/browser/resources/new_incognito_tab_theme.css:10: url(chrome://theme/IDR_THEME_NTP_BACKGROUND@1x?$1) 1x, - After talking to flackr@, we want to change chrome_html.py so that it generates a -webkit-image-set with the "@scalefactor" before the "?".
A few small things https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:1453: void BrowserThemePack::GenerateRawImageForAllScales(int prs_id) { Nit: Rename to GenerateRawImagesForAllSupportedScaleFactors(). https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:1482: int raw_id = GetRawIDByPersistentID(prs_id, scale_factors_[i]); I think you can just go and find the image for |prs_id| with the largest scale factor. The image with the scale factor closest to |highest_missing| is not necessarily the best choice. For instance, an image whose scale factor is an integer multiple of the missing scale factor(s) is a better choice. https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:1519: if (scale_factors_[i] == available) You don't need the if on line 1519 because the if on line 1522 will catch this case too. https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:1525: if (!gfx::PNGCodec::Decode(it->second->front(), You know that you need |available_bitmap| because you exit early on line 1477. Can you move the bitmap initialization outside of the loop?
On 2013/06/07 14:57:15, pkotwicz wrote: > - I think that it is useful to write a CL introducing > IDR_THEME_DETACHED_BOOKMARK_BAR_NTP_BACKGROUND before landing this one. This > guarantees that this CL will not regress performance when the NTP has a detached > bookmark bar. > > - As a heads up, I do not think your CL affects the new tab page when running > with --instant-extended-api (I do not mind if that gets fixed as part of a > separate CL) > > I will continue reviewing the changes to this CL with the above disclaimers > > https://codereview.chromium.org/16610002/diff/1/chrome/browser/resources/new_... > File chrome/browser/resources/new_incognito_tab_theme.css (right): > > https://codereview.chromium.org/16610002/diff/1/chrome/browser/resources/new_... > chrome/browser/resources/new_incognito_tab_theme.css:10: > url(chrome://theme/IDR_THEME_NTP_BACKGROUND@1x?$1) 1x, > - After talking to flackr@, we want to change chrome_html.py so that it > generates a -webkit-image-set with the "@scalefactor" before the "?". RE: IDR_THEME_DETACHED_BOOKMARK_BAR_NTP_BACKGROUND I prefer to keep this improvement for later. This seems complicated. I think the cropping depends on the vertical ntp alignment property (from the theme), i.e. top, center, bottom and on the height on the actual area. (And perhaps also on tiling). And we have to load the uncropped image anyway, wouldn't adding an additional cropped image make the loading slower.
I looks like the alignment properties would make this optimization work only for certain alignment properties. Let's leave it for later
Thanks! RE: -webkit-image-set and changing ./tools/grit/grit/gather/chrome_html.py Can this wait? Or do you suggest to change it with this CL? If so, could you, Rob or someone make the change to chrome_html.py? On Fri, Jun 7, 2013 at 11:18 AM, <pkotwicz@chromium.org> wrote: > I looks like the alignment properties would make this optimization work > only for > certain alignment properties. Let's leave it for later > > > > https://codereview.chromium.**org/16610002/<https://codereview.chromium.org/1... >
I am still concerned as to how this CL will slow down requesting ntp background image via BrowserThemePack::GetImageNamed().
Thanks for all your feedback. https://codereview.chromium.org/16610002/diff/1/chrome/browser/resources/new_... File chrome/browser/resources/new_incognito_tab_theme.css (right): https://codereview.chromium.org/16610002/diff/1/chrome/browser/resources/new_... chrome/browser/resources/new_incognito_tab_theme.css:10: url(chrome://theme/IDR_THEME_NTP_BACKGROUND@1x?$1) 1x, On 2013/06/07 14:57:15, pkotwicz wrote: > - After talking to flackr@, we want to change chrome_html.py so that it > generates a -webkit-image-set with the "@scalefactor" before the "?". Can this CL proceed before that change? https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:1453: void BrowserThemePack::GenerateRawImageForAllScales(int prs_id) { On 2013/06/07 16:15:26, pkotwicz wrote: > Nit: Rename to GenerateRawImagesForAllSupportedScaleFactors(). Done. https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:1482: int raw_id = GetRawIDByPersistentID(prs_id, scale_factors_[i]); On 2013/06/07 16:15:26, pkotwicz wrote: > I think you can just go and find the image for |prs_id| with the largest scale > factor. The image with the scale factor closest to |highest_missing| is not > necessarily the best choice. For instance, an image whose scale factor is an > integer multiple of the missing scale factor(s) is a better choice. Done. https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:1519: if (scale_factors_[i] == available) On 2013/06/07 16:15:26, pkotwicz wrote: > You don't need the if on line 1519 because the if on line 1522 will catch this > case too. Done. https://codereview.chromium.org/16610002/diff/1/chrome/browser/themes/browser... chrome/browser/themes/browser_theme_pack.cc:1525: if (!gfx::PNGCodec::Decode(it->second->front(), On 2013/06/07 16:15:26, pkotwicz wrote: > You know that you need |available_bitmap| because you exit early on line 1477. > Can you move the bitmap initialization outside of the loop? Done.
> RE: -webkit-image-set and changing ./tools/grit/grit/gather/chrome_html.py > > Can this wait? My preference would be to modify the *.css assuming the change in chrome_html.py. And then you could modify chrome_html.py in a separate CL. What you have is fine though modifying the *.css files like this is a bit pointless in my opinion.
GenerateRawImageForAllSupportedScales() looks good https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:587: // any missing scale from an available scale image. We should iterate through kPreloadIDs here. https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:1454: // For any raw theme image for a supported scale that is not in the How about: Compute (by scaling) bitmaps for |prs_id| for any scale factors for which the theme author did not provide a bitmap. We compute the bitmaps using the highest scale factor that theme author provided. Heads up: If the theme author provided images for 1x and 1.8x, the current code will not use the 1.8x image as the source image on CrOS because 1.8x is not supported on CrOS (though it is supported on Windows). And yes, we can handle this case in a followup CL. https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:1486: // Get bitmap of the available image. Nit: How about: Get the bitmap for the available scale factor. https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:1490: DCHECK(it->second->size() > 0); The above DCHECK is not necessary. gfx::PNGCodec::Decode deals with that case https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:1500: // Fill in all missing scales by scaling the available image. Nit scales -> scale factors available image -> available bitmap I try not to mix image & bitmap in comments because image usually implies a gfx::Image/gfx::ImageSkia and bitmap usually implies SkBitmap.
LGTM
https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... File chrome/browser/themes/browser_theme_pack.cc (right): https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:587: // any missing scale from an available scale image. On 2013/06/10 15:20:11, pkotwicz wrote: > We should iterate through kPreloadIDs here. Done. https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:1454: // For any raw theme image for a supported scale that is not in the On 2013/06/10 15:20:11, pkotwicz wrote: > How about: > Compute (by scaling) bitmaps for |prs_id| for any scale factors for which the > theme author did not provide a bitmap. We compute the bitmaps using the highest > scale factor that theme author provided. > > Heads up: > If the theme author provided images for 1x and 1.8x, the current code will not > use the 1.8x image as the source image on CrOS because 1.8x is not supported on > CrOS (though it is supported on Windows). > And yes, we can handle this case in a followup CL. Done. https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:1486: // Get bitmap of the available image. On 2013/06/10 15:20:11, pkotwicz wrote: > Nit: How about: > Get the bitmap for the available scale factor. Done. https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:1490: DCHECK(it->second->size() > 0); On 2013/06/10 15:20:11, pkotwicz wrote: > The above DCHECK is not necessary. gfx::PNGCodec::Decode deals with that case Actually, we don't need either one. Done. https://codereview.chromium.org/16610002/diff/10001/chrome/browser/themes/bro... chrome/browser/themes/browser_theme_pack.cc:1500: // Fill in all missing scales by scaling the available image. On 2013/06/10 15:20:11, pkotwicz wrote: > Nit scales -> scale factors > available image -> available bitmap > > I try not to mix image & bitmap in comments because image usually implies a > gfx::Image/gfx::ImageSkia and bitmap usually implies SkBitmap. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/16610002/19002
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Updated *.css for NTP. This should be their final form. Correct behavior depends on https://code.google.com/p/chromium/issues/detail?id=248189 (coming soon).
Still LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/16610002/33001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Added reviewer xiyuan@ for *.css, *.js
css/js LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/16610002/33001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sschmitz@chromium.org/16610002/33001
Message was sent while issue was closed.
Change committed as 205862 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
