Closed
Bug 1258524
Opened 8 years ago
Closed 8 years ago
Pocket icon/button gone after 46.0 on Unix != Linux
Categories
(Firefox :: Pocket, defect)
Tracking
()
VERIFIED
FIXED
Firefox 48
People
(Reporter: jbeich, Assigned: mixedpuppy)
References
Details
Attachments
(2 files)
1.31 KB,
image/png
|
Details | |
1.16 KB,
patch
|
Gijs
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1257947 +++ Similar to bug 1257947 chrome registration in Pocket extension needs to be platform- [1] rather than OS_TARGET -specific. JS code (via AppConstants.platform) already treats any GTK system as Linux. The following line works fine here with os=FreeBSD. # browser/extensions/pocket/jar.mn % skin pocket classic/1.0 %skin/linux/ os=Linux [1] https://developer.mozilla.org/en-US/docs/Chrome_Registration#platform_.28Platform-specific_packages.29
Summary: Pocket icon/button gone after 45.0 on Unix != Linux → Pocket icon/button gone after 46.0 on Unix != Linux
Comment 1•8 years ago
|
||
(In reply to Jan Beich from comment #0) > Created attachment 8733045 [details] > Nightly screenshot with missing/broken buttons > > +++ This bug was initially created as a clone of Bug #1257947 +++ > > Similar to bug 1257947 chrome registration in Pocket extension needs to be > platform- [1] rather than OS_TARGET -specific. JS code (via > AppConstants.platform) already treats any GTK system as Linux. The following > line works fine here with os=FreeBSD. > > # browser/extensions/pocket/jar.mn > % skin pocket classic/1.0 %skin/linux/ os=Linux > > [1] > https://developer.mozilla.org/en-US/docs/Chrome_Registration#platform_. > 28Platform-specific_packages.29 Shane, same question as bug 1257947 comment 1 - maybe we could let the Linux skin be the fallback (no "os=Linux" in the chrome manifest)?
Flags: needinfo?(mixedpuppy)
Assignee | ||
Comment 2•8 years ago
|
||
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Attachment #8733154 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 3•8 years ago
|
||
[Tracking Requested - why for this release]: addon manifest not correct for *unix*, skin will be wrong, resulting in missing icon.
tracking-firefox46:
--- → ?
Assignee | ||
Updated•8 years ago
|
Comment 4•8 years ago
|
||
Comment on attachment 8733154 [details] [diff] [review] fix icon for unix Review of attachment 8733154 [details] [diff] [review]: ----------------------------------------------------------------- r=me like this or with the suggestion below. ::: browser/extensions/pocket/jar.mn @@ +8,2 @@ > % skin pocket classic/1.0 %skin/osx/ os=Darwin > % skin pocket classic/1.0 %skin/windows/ os=WINNT Because this file is preprocessed by necessity, maybe we can use the same logic we use for the toolkit/browser themes? That would avoid any other surprises here, or (like this patch) depend on duplicate skin package registrations overriding each other...
Attachment #8733154 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #4) > Comment on attachment 8733154 [details] [diff] [review] > fix icon for unix > > Review of attachment 8733154 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me like this or with the suggestion below. > > ::: browser/extensions/pocket/jar.mn > @@ +8,2 @@ > > % skin pocket classic/1.0 %skin/osx/ os=Darwin > > % skin pocket classic/1.0 %skin/windows/ os=WINNT > > Because this file is preprocessed by necessity, maybe we can use the same > logic we use for the toolkit/browser themes? That would avoid any other > surprises here, or (like this patch) depend on duplicate skin package > registrations overriding each other... IIUC you mean to have a separate jar.mn for each theme or platform, then these are determined at build time rather than runtime. We're going to have to depend on multiple platform support in the addons anyway, so I think it's probably best to keep the multiple platform support in there.
Comment 6•8 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #5) > (In reply to :Gijs Kruitbosch from comment #4) > > Comment on attachment 8733154 [details] [diff] [review] > > fix icon for unix > > > > Review of attachment 8733154 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > r=me like this or with the suggestion below. > > > > ::: browser/extensions/pocket/jar.mn > > @@ +8,2 @@ > > > % skin pocket classic/1.0 %skin/osx/ os=Darwin > > > % skin pocket classic/1.0 %skin/windows/ os=WINNT > > > > Because this file is preprocessed by necessity, maybe we can use the same > > logic we use for the toolkit/browser themes? That would avoid any other > > surprises here, or (like this patch) depend on duplicate skin package > > registrations overriding each other... > > > IIUC you mean to have a separate jar.mn for each theme or platform, then > these are determined at build time rather than runtime. > > We're going to have to depend on multiple platform support in the addons > anyway, so I think it's probably best to keep the multiple platform support > in there. No, I meant something like: #ifdef XP_WIN % skin pocket classic/1.0 %skin/windows/ #elif XP_MACOSX % skin pocket classic/1.0 %skin/osx/ #else % skin pocket classic/1.0 %skin/linux/ #endif Does that make more sense?
Assignee | ||
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/fb255ce8318d62a75ee0c671809cd0cf8baaab0a Bug 1258524 fix icon for *unix*, r=gijs
Comment 8•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb255ce8318d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8733154 [details] [diff] [review] fix icon for unix Approval Request Comment [Feature/regressing bug #]:pocket [User impact if declined]:no icon in button on unix platforms (ie. non-linux) [Describe test coverage new/current, TreeHerder]:manual verification [Risks and why]: this causes a change to the addon manifest, no code changes, should only have an effect on unix platforms. [String/UUID change made/needed]:none
Attachment #8733154 -
Flags: approval-mozilla-beta?
Attachment #8733154 -
Flags: approval-mozilla-aurora?
Comment 11•8 years ago
|
||
Comment on attachment 8733154 [details] [diff] [review] fix icon for unix Pocket should work for linux, let's take this for beta 5
Attachment #8733154 -
Flags: approval-mozilla-beta?
Attachment #8733154 -
Flags: approval-mozilla-beta+
Attachment #8733154 -
Flags: approval-mozilla-aurora?
Attachment #8733154 -
Flags: approval-mozilla-aurora+
Comment 12•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/732c1dd6c6dc
Reporter | ||
Comment 13•8 years ago
|
||
Confirmed the icon re-appeared on Nightly. For Beta I'll probably wait closer to release and do after rebasing patches (for other bugs) in www/firefox port.
Comment 14•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/f1e5f0b895df
Comment 15•8 years ago
|
||
[bugday-20160323] Able to get expected results for: Status: RESOLVED,FIXED --> VERIFIED Component: Name Firefox Version 46.0b4 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 Actual Results: As expected Expected Results: Able to see and access pocket button.
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•