options/guest mode: only hide the 'Show advanced settings...' link when

options/guest mode: only hide the 'Show advanced settings...' link when
... there's no advanced settings.

R=mlerman@chromium.org
BUG=452804
Committed: https://crrev.com/8220d7f821a93930809000d9ee88462d2b164890
Cr-Commit-Position: refs/heads/master@{#313843}

[Mike Lerman, 2015-01-29 17:08:06]

lgtm

[Dan Beam, 2015-01-29 17:38:48]

ok, this seems like the best approach, ptal

[Mike Lerman, 2015-01-29 17:56:44]

https://codereview.chromium.org/886843002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/886843002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:1000: <footer
id="advanced-settings-footer">
Why give the footer an id? Can't you jquery-select the footer with just
$('footer')?

[Dan Beam, 2015-01-29 20:07:16]

https://codereview.chromium.org/886843002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.html (right):

https://codereview.chromium.org/886843002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.html:1000: <footer
id="advanced-settings-footer">
On 2015/01/29 17:56:44, Mike Lerman wrote:
> Why give the footer an id? Can't you jquery-select the footer with just
> $('footer')?

$() is document.getElementById, not jQuery's sizzle.

I added an ID because doing

  document.querySelector('footer').hidden = true;

breaks if another <footer> is added above it in the page.

  var footers = document.querySelectorAll('footer');
  assert(footers.length == 1);
  footers[0].hidden = true;

is brittle and overkill.

  document.querySelector('#advanced-settings + footer').hidden = true;

breaks if they're not direct siblings and

  document.querySelector('#advanced-settings ~ footer').hidden = true;

is fragile if there's another <footer> added between them.

adding an ID is the only way that doesn't break with a different DOM.

[Mike Lerman, 2015-01-29 21:29:42]

On 2015/01/29 20:07:16, Dan Beam wrote:
>
https://codereview.chromium.org/886843002/diff/60001/chrome/browser/resources...
> File chrome/browser/resources/options/browser_options.html (right):
> 
>
https://codereview.chromium.org/886843002/diff/60001/chrome/browser/resources...
> chrome/browser/resources/options/browser_options.html:1000: <footer
> id="advanced-settings-footer">
> On 2015/01/29 17:56:44, Mike Lerman wrote:
> > Why give the footer an id? Can't you jquery-select the footer with just
> > $('footer')?
> 
> $() is document.getElementById, not jQuery's sizzle.
> 
> I added an ID because doing
> 
>   document.querySelector('footer').hidden = true;
> 
> breaks if another <footer> is added above it in the page.
> 
>   var footers = document.querySelectorAll('footer');
>   assert(footers.length == 1);
>   footers[0].hidden = true;
> 
> is brittle and overkill.
> 
>   document.querySelector('#advanced-settings + footer').hidden = true;
> 
> breaks if they're not direct siblings and
> 
>   document.querySelector('#advanced-settings ~ footer').hidden = true;
> 
> is fragile if there's another <footer> added between them.
> 
> adding an ID is the only way that doesn't break with a different DOM.

slgtm :)

[Dan Beam, 2015-01-29 23:56:49]

The CQ bit was checked by dbeam@chromium.org

[commit-bot: I haz the power, 2015-01-29 23:57:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/886843002/60001

[commit-bot: I haz the power, 2015-01-30 00:51:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-01-30 00:53:38]

Patchset 4 (id:??) landed as
https://crrev.com/8220d7f821a93930809000d9ee88462d2b164890
Cr-Commit-Position: refs/heads/master@{#313843}