Support extension highlighting on the extensions page.

Support extension highlighting on the extensions page.

BUG=153137
TEST=Add a ?id=<extension_id> to the chrome://extensions page url and that extension should become highlighted.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170561

[Finnur, 2012-11-29 04:07:33]

https://codereview.chromium.org/11434024/diff/1/chrome/browser/resources/exte...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/11434024/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_list.js:55:
$(id_to_highlight).scrollIntoView();
After fighting with this for a while I'm ready to pull the plug on this
scrolling, since it doesn't work too well -- items scrolled to are usually
partly off-screen. :/

[Aaron Boodman, 2012-11-29 21:39:40]

Seems reasonable to me.

https://codereview.chromium.org/11434024/diff/1/chrome/browser/resources/exte...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/11434024/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_list.js:55:
$(id_to_highlight).scrollIntoView();
On 2012/11/29 04:07:34, Finnur wrote:
> After fighting with this for a while I'm ready to pull the plug on this
> scrolling, since it doesn't work too well -- items scrolled to are usually
> partly off-screen. :/

Is it because of the heading layer? You can get the actual scroll offset with
element.scrollTop and then add the height of the heading with
headingElement.offsetHeight. You'd probably need some spacing too.

https://codereview.chromium.org/11434024/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_list.js:296:
extensionIdToHighlight: function() {
See parseQueryParams() in browser/resources/shared/js/util.js

[Finnur, 2012-11-30 19:12:45]

I converted it to use parseQueryParams, changed the highlighting to look like
the mock and changed the scrolling to that the scrolled item would not be
completely at the top (it makes you feel like the extensions above you are
non-existent). Having the extension above the one that is highlighted partially
visible reduces the cognitive dissonance from the auto-scrolling a bit. PTAL

[Finnur, 2012-11-30 19:18:35]

s/to that/so that/.

On 2012/11/30 19:12:45, Finnur wrote:
> I converted it to use parseQueryParams, changed the highlighting to look like
> the mock and changed the scrolling to that the scrolled item would not be
> completely at the top (it makes you feel like the extensions above you are
> non-existent). Having the extension above the one that is highlighted
partially
> visible reduces the cognitive dissonance from the auto-scrolling a bit. PTAL

[Aaron Boodman, 2012-11-30 19:49:23]

lgtm

[Aaron Boodman, 2012-11-30 20:07:33]

+jhawkins for OWNERS

[James Hawkins, 2012-11-30 20:11:05]

LGTM with nits.

https://codereview.chromium.org/11434024/diff/9001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/11434024/diff/9001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:60: (1.2 *
$(id_to_highlight).clientHeight);
nit: Pull 1.2 out into a constant.

https://codereview.chromium.org/11434024/diff/9001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:90: var id_to_highlight =
parseQueryParams(document.location)['id'];
Optional nit: Pull parseQueryParams...; out into a helper method to reduce
fragility, e.g., getIdQueryParam() or something.

[Finnur, 2012-11-30 21:49:28]

Thanks for taking a look James.

https://codereview.chromium.org/11434024/diff/9001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_list.js (right):

https://codereview.chromium.org/11434024/diff/9001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:60: (1.2 *
$(id_to_highlight).clientHeight);
const are not allowed in strict mode, but I factored this out of the equation. 

On 2012/11/30 20:11:05, James Hawkins wrote:
> nit: Pull 1.2 out into a constant.

https://codereview.chromium.org/11434024/diff/9001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_list.js:90: var id_to_highlight =
parseQueryParams(document.location)['id'];
On 2012/11/30 20:11:05, James Hawkins wrote:
> Optional nit: Pull parseQueryParams...; out into a helper method to reduce
> fragility, e.g., getIdQueryParam() or something.

Done.