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)

Unspecified
FreeBSD
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox46 + verified
firefox47 --- verified
firefox48 --- verified

People

(Reporter: jbeich, Assigned: mixedpuppy)

References

Details

Attachments

(2 files)

+++ 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
(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: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Attachment #8733154 - Flags: review?(gijskruitbosch+bugs)
[Tracking Requested - why for this release]: addon manifest not correct for *unix*, skin will be wrong, resulting in missing icon.
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+
(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.
(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?
https://hg.mozilla.org/mozilla-central/rev/fb255ce8318d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
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?
Tracking since this is a regression
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+
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.
[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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.