Issue 3887 - Support bringing up the new options page with a specific tab focused

Issue 3887 - Support bringing up the new options page with a specific tab focused

[Thomas Greiner, 2016-04-04 12:46:36]

https://codereview.adblockplus.org/29339320/diff/29339341/options.js
File options.js (right):

https://codereview.adblockplus.org/29339320/diff/29339341/options.js#newcode1138
options.js:1138: case "switchToOptionsSection":
Detail: Is this specific to the options page? We could make this more generic to
account for a more interesting UX.

e.g. Calling it "focusSection" would put the focus on a section on the page the
message sent to and imply all of the necessary actions to achieve that such as
switching tabs, opening dialogs and highlighting sections.

Note that in the current options page the message type is called "focus-section"
so we could reuse that (and maybe rename "addSubscription" to "add-subscription"
for consistency). There it also not only switches to the tab but highlights the
section as well (something that's missing from this implementation).

[Sebastian Noack, 2016-04-05 13:32:02]

https://codereview.adblockplus.org/29339320/diff/29339341/options.js
File options.js (right):

https://codereview.adblockplus.org/29339320/diff/29339341/options.js#newcode1138
options.js:1138: case "switchToOptionsSection":
On 2016/04/04 12:46:36, Thomas Greiner wrote:
> Detail: Is this specific to the options page? We could make this more generic
to
> account for a more interesting UX.
> 
> e.g. Calling it "focusSection" would put the focus on a section on the page
the
> message sent to and imply all of the necessary actions to achieve that such as
> switching tabs, opening dialogs and highlighting sections.

Fine with me using a more generic name that doesn't imply the options page. Any
preference: switchToSection, switchToTab, focusSection, focusTab?

> Note that in the current options page the message type is called
"focus-section"
> so we could reuse that (and maybe rename "addSubscription" to
"add-subscription"
> for consistency).

This is going to change with the current rewrite of the old options page anyway,
to comply with whatever we are going to use here. As for consistency, I'd prefer
to stick to the current scheme used by the new options page.

> section as well (something that's missing from this implementation).

It seems that those aren't working currently anyway. FWIW, I think just
switching to the corresponding tab is good enough. Not that we ever used those
notifications anyway (except for one test with a limited user base).

[Thomas Greiner, 2016-04-06 12:49:13]

https://codereview.adblockplus.org/29339320/diff/29339341/options.js
File options.js (right):

https://codereview.adblockplus.org/29339320/diff/29339341/options.js#newcode1138
options.js:1138: case "switchToOptionsSection":
On 2016/04/05 13:32:02, Sebastian Noack wrote:
> Fine with me using a more generic name that doesn't imply the options page.
Any
> preference: switchToSection, switchToTab, focusSection, focusTab?

I'd suggest "focusSection" to no limit ourselves to tabs.

> This is going to change with the current rewrite of the old options page
anyway,
> to comply with whatever we are going to use here. As for consistency, I'd
prefer
> to stick to the current scheme used by the new options page.

Ok, that's fine.

> It seems that those aren't working currently anyway. FWIW, I think just
> switching to the corresponding tab is good enough.

Should be fine for now, I agree. It's something we can implement later on when
we use it in some more common feature.

[Sebastian Noack, 2016-04-06 17:38:29]

https://codereview.adblockplus.org/29339320/diff/29339341/options.js
File options.js (right):

https://codereview.adblockplus.org/29339320/diff/29339341/options.js#newcode1138
options.js:1138: case "switchToOptionsSection":
On 2016/04/06 12:49:12, Thomas Greiner wrote:
> On 2016/04/05 13:32:02, Sebastian Noack wrote:
> > Fine with me using a more generic name that doesn't imply the options page.
> Any
> > preference: switchToSection, switchToTab, focusSection, focusTab?
> 
> I'd suggest "focusSection" to no limit ourselves to tabs.

Done.

https://codereview.adblockplus.org/29339320/diff/29339525/options.js
File options.js (right):

https://codereview.adblockplus.org/29339320/diff/29339525/options.js#newcode924
options.js:924: getPref("subscriptions_exceptionsurl", function(value)
Please ignore this change. It's not part of the patch but due to rebasing.

[Thomas Greiner, 2016-04-06 18:05:08]

LGTM