Waiting to land bug 1215694 before handling this to avoid potential bitrot on the image patch.

copies pocket icon to addon. - images created by copying originals and croping to only have pocket icon - duplicated chrome.manifest overrides to maintain skin appearance - have not visually inspected all variants (I just don't have them) - not removing pocket icon from browser skin yet (will followup with 2nd patch/bug)

Comment on attachment 8707228 [details] [diff] [review] pocketicon Review of attachment 8707228 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/extensions/pocket/jar.mn @@ +17,5 @@ > +% override chrome://pocket/skin/menuPanel@2x.png chrome://pocket/skin/menuPanel-aero@2x.png os=WINNT osversion=6.1 > +% override chrome://pocket/skin/menuPanel-small.png chrome://pocket/skin/menuPanel-small-aero.png os=WINNT osversion=6 > +% override chrome://pocket/skin/menuPanel-small.png chrome://pocket/skin/menuPanel-small-aero.png os=WINNT osversion=6.1 > +% override chrome://pocket/skin/menuPanel-small@2x.png chrome://pocket/skin/menuPanel-small-aero@2x.png os=WINNT osversion=6 > +% override chrome://pocket/skin/menuPanel-small@2x.png chrome://pocket/skin/menuPanel-small-aero@2x.png os=WINNT osversion=6.1 This looks like copy-pasta soup - you don't need/use menuPanel-small.png, right? Conversely, you added lunaSilver.png, but there seems to be no override for it. ::: browser/extensions/pocket/skin/osx/pocket.css @@ +34,5 @@ > } > > @media (min-resolution: 1.1dppx) { > #pocket-button:hover:active:not([disabled="true"]):not([cui-areatype="menu-panel"]) { > + -moz-image-region: rect(36px, 18px, 72px, 0); This is wrong, isn't it? Should be 2x the values in the previous selector. ::: browser/extensions/pocket/skin/shared/pocket.css @@ +14,5 @@ > } > > #pocket-button { > + list-style-image: url("chrome://pocket/skin/Toolbar.png"); > + -moz-image-region: rect(0, 18px, 18px, 0); Nit: indentation, here and elsewhere

(In reply to :Gijs Kruitbosch from comment #2) > Conversely, you added lunaSilver.png, but there seems to be no override for > it. it's added via css (but I still missed that)

Review commit: https://reviewboard.mozilla.org/r/30945/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30945/

Comment on attachment 8708027 [details] MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?gijs https://reviewboard.mozilla.org/r/30945/#review27789 So, I'm sorry for not realizing this the first time, but... can you crop the images to use 16x16 and 32x32px for Toolbar.png, and use those coordinate multipliers? Because the icon is clearly being rendered inside 16px, and there's 1px of empty space on all sides, but because we're sizing the 18x18 pixels to 16x16, it looks squashed. Finally, can you use a tool like ImageOptim.app ? On these files, the savings are on average some 69% (!). ::: browser/extensions/pocket/skin/shared/pocket.css:23 (Diff revision 1) > + -moz-image-region: rect(0, 18px, 18px, 0); You shouldn't need to re-set the moz-image-region here, right?

I thought these were all supposed to be 18px. The are cropped out of the existing files at the same size, and toolbarbuttons.inc.css uses 18px offsets for everything.

also, it does seem the icons are smaller than they are supposed to be, I'm not clear why, was going to look into that.

(In reply to Shane Caraveo (:mixedpuppy) from comment #7) > Created attachment 8708124 [details] > pocketiconsize.png > > also, it does seem the icons are smaller than they are supposed to be, I'm > not clear why, was going to look into that. It's late here, but see comments on bug 1233701. If that's not clear, I can clarify more in the morning.

While normal are 16px with padding in the image, inverted icons are actually 18px. Does this also affect @2x icons?

(In reply to Shane Caraveo (:mixedpuppy) from comment #9) > While normal are 16px with padding in the image, inverted icons are actually > 18px. Does this also affect @2x icons? Yes. If the inverted icons are too big, then the other option is to override the styles for #pocket-button, but that's going to be "interesting". That or get the inverted icons resized by UX. :-(

Simply adding the css below fixes the problem so far as I can see. Works in toolbar, overflow, doesn't affect palette or menu. #pocket-button[cui-areatype="toolbar"] > .toolbarbutton-icon { max-width: 18px; margin: 0; } Is there something I'm missing?

(In reply to Shane Caraveo (:mixedpuppy) from comment #11) > Simply adding the css below fixes the problem so far as I can see. Works in > toolbar, overflow, doesn't affect palette or menu. > > #pocket-button[cui-areatype="toolbar"] > .toolbarbutton-icon { > max-width: 18px; > margin: 0; > } > > Is there something I'm missing? Did you test cross-platform? Off the top of my head, I think the padding will still be off with this CSS.

(In reply to :Gijs Kruitbosch from comment #12) > > Is there something I'm missing? > > Did you test cross-platform? > > Off the top of my head, I think the padding will still be off with this CSS. tested only on osx at this point, padding is fine. I was just wondering if I'm missing some bit of complexity based on your "interesting" comment 10.

(In reply to Shane Caraveo (:mixedpuppy) from comment #13) > (In reply to :Gijs Kruitbosch from comment #12) > > > > Is there something I'm missing? > > > > Did you test cross-platform? > > > > Off the top of my head, I think the padding will still be off with this CSS. > > tested only on osx at this point, padding is fine. It's Windows and Linux where the max-width includes a sizable amount of padding so that you can click "outside" of the border of the button and still hit it. That is where this will break. See e.g. https://dxr.mozilla.org/mozilla-central/rev/b67316254602a63bf4e568198a5c7d3288a9db27/browser/themes/linux/browser.css#605-609 . > I was just wondering if > I'm missing some bit of complexity based on your "interesting" comment 10. Making sure that this works correctly in all of the OSes and devedition, and irrespective of the button's placement etc. is tricky. Hard to verify when writing the patch, hard to review.

Comment on attachment 8708027 [details] MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?gijs Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30945/diff/1-2/

Verified on win/lin/osx but I don't have the screen for @2x

Comment on attachment 8708027 [details] MozReview Request: Bug 1230638 move pocket toolbar/menu icons into addon, r?gijs https://reviewboard.mozilla.org/r/30945/#review28645 Tested for hidpi on OSX and Windows, looks OK to me. FWIW, you can test the hidpi case by just setting the layout.css.devPixelsPerPx pref to 2.0 on OSX and something like 1.25 on Windows. :-) ::: browser/extensions/pocket/skin/windows/pocket.css:3 (Diff revisions 1 - 2) > +#nav-bar #pocket-button > .toolbarbutton-icon, > +#nav-bar #pocket-button > .toolbarbutton-badge-stack, > +#nav-bar #pocket-button > .toolbarbutton-menubutton-button > .toolbarbutton-icon { The first 1 of 3 ::: browser/extensions/pocket/skin/windows/pocket.css:9 (Diff revisions 1 - 2) > +:-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > .toolbarbutton-icon, > +:-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > > +#pocket-button:-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon { And the first 2 of 3 ::: browser/extensions/pocket/skin/linux/pocket.css:3 (Diff revision 2) > +#nav-bar #pocket-button > .toolbarbutton-icon, > +#nav-bar #pocket-button > .toolbarbutton-badge-stack, > +#nav-bar #pocket-button > .toolbarbutton-menubutton-button > .toolbarbutton-icon { You only need the first of these three selectors. ::: browser/extensions/pocket/skin/linux/pocket.css:9 (Diff revision 2) > +:-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > .toolbarbutton-icon, > +:-moz-any(#TabsToolbar, .widget-overflow-list) #pocket-button > > +#pocket-button:-moz-any(.toolbarbutton-menubutton-button, .toolbarbutton-badge-stack) > .toolbarbutton-icon { The first 2 of the 3...

Backed out in https://hg.mozilla.org/integration/fx-team/rev/ab602ecfad88 - it's a fair bet that Linux32 debug and ASan are our slowest two platforms, and they said https://treeherder.mozilla.org/logviewer.html#?job_id=6785463&repo=fx-team 2-of-10 times on Linux32 debug and https://treeherder.mozilla.org/logviewer.html#?job_id=6786378&repo=fx-team 8-of-10 times on ASan.

[bugday-20160323] Status: RESOLVED,FIXED -> INCOMPLETE Comments: STR: It is not recommend to mess with user privacy and choice by giving private access to Pocket addon. Component: Name Firefox Version 46.0b9 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Expected Results: pocket addon instead of icon. Actual Results: As expected