Add the sidebar button into the default toolbar set

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
3 months ago
20 days ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
As per https://mozilla.invisionapp.com/share/XRBHN5SWD#/screens, the sidebar button will appear by default in the toolbar.

Updated

3 months ago
Flags: qe-verify?
Priority: -- → P2

Updated

3 months ago
Component: General → Toolbars and Customization
Flags: qe-verify? → qe-verify+

Updated

3 months ago
QA Contact: gwimberly
(Assignee)

Updated

2 months ago
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Updated

2 months ago
Priority: P2 → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 2

2 months ago
I feel like this is missing something but I'm not sure what. I don't see any related failures on a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=921701d08c17de43f3b4edb9a43fe2129e46cf37&selectedJob=106828662

Comment 3

2 months ago
mozreview-review
Comment on attachment 8877749 [details]
Bug 1364238 - Add the sidebar button into the default toolbar set;

https://reviewboard.mozilla.org/r/149178/#review153916

I think you also need to update BrowserUITelemetry. Which looks like it already missed an update for the social share button, and maybe other stuff? Maybe we should put some effort in to make it rely on default placements as reported by CUI directly instead of boilerplating all the things... doesn't need to happen in this bug though.
Attachment #8877749 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

2 months ago
See Also: → bug 1374709
(Assignee)

Comment 4

2 months ago
(In reply to :Gijs from comment #3)
> Comment on attachment 8877749 [details]
> Bug 1364238 - Add the sidebar button into the default toolbar set;
> 
> https://reviewboard.mozilla.org/r/149178/#review153916
> 
> I think you also need to update BrowserUITelemetry. Which looks like it
> already missed an update for the social share button, and maybe other stuff?
> Maybe we should put some effort in to make it rely on default placements as
> reported by CUI directly instead of boilerplating all the things... doesn't
> need to happen in this bug though.

Filed Bug 1374709 for that
Comment hidden (mozreview-request)

Comment 6

2 months ago
mozreview-review
Comment on attachment 8877749 [details]
Bug 1364238 - Add the sidebar button into the default toolbar set;

https://reviewboard.mozilla.org/r/149178/#review155768

Assuming this is green on try, r=me
Attachment #8877749 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 7

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49c1de18e3a27bbeb9bb8dd86adfd55335d45b0d

Comment 8

2 months ago
mozreview-review
Comment on attachment 8877749 [details]
Bug 1364238 - Add the sidebar button into the default toolbar set;

https://reviewboard.mozilla.org/r/149178/#review155770

Ah, oh, so, one more thing... presumably we want to add this to the navbar for existing users? This patch doesn't do that yet. Sorry, I should have thought about this in my first review. :-\
Attachment #8877749 - Flags: review+

Comment 9

2 months ago
Though maybe we want to postpone doing that until 57? Or something? I dunno. We don't have good mechanisms for doing this conditionally on nightly now but not incrementing some kind of count that would then be out-of-sync when we take it out from MOZ_PHOTON_THEME in the 57 cycle. :-\
(Assignee)

Comment 10

2 months ago
(In reply to :Gijs from comment #9)
> Though maybe we want to postpone doing that until 57? Or something? I dunno.
> We don't have good mechanisms for doing this conditionally on nightly now
> but not incrementing some kind of count that would then be out-of-sync when
> we take it out from MOZ_PHOTON_THEME in the 57 cycle. :-\

So the problem is that we may not want to migrate users immediately because AppConstants.MOZ_PHOTON_THEME will still be off by default in Beta 56?  And we could do a migration only if AppConstants.MOZ_PHOTON_THEME is true so it won't accidentally migrate Beta users to have the sidebar button by default, but also means that when we do flip that flag those same users *won't* get the migration because they've already had the chance and missed it.

So we need to either not do a migration now and instead wait until 57 (which means current nightly users won't get the change) or do the migration now conditionally *and* do it again in 57 (which means current nightly users will get the change, although if they then remove the sidebar it might pop back up again with the new migration).

I'm inclined to file a new bug for the migration and land it at the beginning of 57 to avoid doing it twice.
(Assignee)

Comment 11

2 months ago
Is Comment 10 a correct summary of the situation, and do you have an opinion?
Flags: needinfo?(gijskruitbosch+bugs)

Comment 12

2 months ago
(In reply to Brian Grinstead [:bgrins] from comment #10)
> I'm inclined to file a new bug for the migration and land it at the
> beginning of 57 to avoid doing it twice.

Yeah, that would be the simplest.
Flags: needinfo?(gijskruitbosch+bugs)

Comment 13

2 months ago
mozreview-review
Comment on attachment 8877749 [details]
Bug 1364238 - Add the sidebar button into the default toolbar set;

https://reviewboard.mozilla.org/r/149178/#review156084
Attachment #8877749 - Flags: review+
(Assignee)

Updated

2 months ago
Blocks: 1375080

Comment 14

2 months ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c4c815e1ad5
Add the sidebar button into the default toolbar set;r=Gijs

Comment 15

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5c4c815e1ad5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56

Updated

2 months ago
Iteration: --- → 56.1 - Jun 26

Comment 16

2 months ago
"Sidebar button into the default toolbar set" feature has been implemented with Latest Nightly 56.0a1 on Windows 8.1 (64 bit).

Build ID  : 20170623045418
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170621]
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+

Updated

21 days ago
Depends on: 1387081
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.