MD Settings: Highlight sub-page search hits.

MD Settings: Highlight sub-page search hits.

Specifically:
 - When a search hit occurs in a subpage, display a search bubble
   next to the element that is desginated as the "associated control"
   The search bubble guides the user to find the search hit.
 - Address case where multiple search hits occur in the same supgage,
   by ensuring that only one search bubble is added.
 - Update "unhighlight" codepath to also remove search bubbles.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/52c0e1b022a26550030b711b7e2019a4a0bfa3d4
Cr-Commit-Position: refs/heads/master@{#410136}

[dpapad, 2016-08-02 18:25:59]

Description was changed from

==========
MD Settings: Highlight sub-level hits, WIP.

BUG=
==========

to

==========
MD Settings: Highlight sub-level hits, WIP.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-03 18:56:44]

Description was changed from

==========
MD Settings: Highlight sub-level hits, WIP.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Highlight sub-level hits, WIP.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-03 21:56:43]

Description was changed from

==========
MD Settings: Highlight sub-level hits, WIP.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Highlight sub-level hits.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-03 22:05:05]

Patchset #4 (id:60001) has been deleted

[dpapad, 2016-08-03 22:05:10]

Patchset #4 (id:80001) has been deleted

[dpapad, 2016-08-03 22:13:41]

Description was changed from

==========
MD Settings: Highlight sub-level hits.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Highlight sub-page search hits.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-04 21:15:56]

Patchset #6 (id:140001) has been deleted

[dpapad, 2016-08-04 21:16:00]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-08-04 21:16:05]

Patchset #1 (id:20001) has been deleted

[dpapad, 2016-08-04 21:16:10]

Patchset #1 (id:40001) has been deleted

[dpapad, 2016-08-04 21:16:14]

Patchset #1 (id:100001) has been deleted

[dpapad, 2016-08-04 22:33:40]

Patchset #2 (id:160001) has been deleted

[dpapad, 2016-08-04 22:34:58]

Patchset #2 (id:180001) has been deleted

[dpapad, 2016-08-04 22:37:51]

Description was changed from

==========
MD Settings: Highlight sub-page search hits.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Highlight sub-page search hits.

Specifically:
 - When a search hit occurs in a subpage, display a search bubble
   next to the element that is desginated as the "associated control"
   The search bubble guides the user to find the search hit.
 - Address case where multiple search hits occur in the same supgage,
   by ensuring that only one search bubble is added.
 - Update "unhighlight" codepath to also remove search bubbles.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-04 22:39:49]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[dpapad, 2016-08-04 22:39:50]

Screenshots at http://imgur.com/a/BAQdj.

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:201: // TODO(dpapad): Cast
to SettingsSubpageElement here.
Tried to address this TODO by casting to SettingsSubpageElement and updating the
compiled_resources2.gyp file accordingly. Unfortunately it results in a
seemingly unrelated compiler error (pasting below),

(ERROR) Error in: settings_main.js
##
/usr/local/google/home/dpapad/workspace/chromium1/src/third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple-extracted.js:545:
ERROR - mismatch of the animate property type and the type of the property it
overrides from superclass Element
## original: function (this:Animatable, (Object|null),
(KeyframeEffectOptions|number)=): Animation
## override: function (this:PaperRippleElement): undefined
##       animate: function() {
##                ^

[Dan Beam, 2016-08-05 00:07:59]

lgtm

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:201: // TODO(dpapad): Cast
to SettingsSubpageElement here.
On 2016/08/04 22:39:49, dpapad wrote:
> Tried to address this TODO by casting to SettingsSubpageElement and updating
the
> compiled_resources2.gyp file accordingly. Unfortunately it results in a
> seemingly unrelated compiler error (pasting below),
> 
> (ERROR) Error in: settings_main.js
> ##
>
/usr/local/google/home/dpapad/workspace/chromium1/src/third_party/polymer/v1_0/components-chromium/paper-ripple/paper-ripple-extracted.js:545:
> ERROR - mismatch of the animate property type and the type of the property it
> overrides from superclass Element
> ## original: function (this:Animatable, (Object|null),
> (KeyframeEffectOptions|number)=): Animation
> ## override: function (this:PaperRippleElement): undefined
> ##       animate: function() {
> ##                ^

don't really understand why this would happen, but I'll take your word for it

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:207: associatedControl =
parent.associatedControl;
nit:

  associatedControl = assert(parent.associatedControl, ...);

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:357: height: 28px;
what if the font-size changes?  does 28px still work?

[dpapad, 2016-08-05 01:33:35]

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:201: // TODO(dpapad): Cast
to SettingsSubpageElement here.
##                ^
> 
> don't really understand why this would happen, but I'll take your word for it

I don't understand either. It is a bit concerning, since presumably the file
that throws the error is already compiled through other targets (but does not
throw any errors?).

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/search_settings.js:207: associatedControl =
parent.associatedControl;
On 2016/08/05 at 00:07:59, Dan Beam wrote:
> nit:
> 
>   associatedControl = assert(parent.associatedControl, ...);

Done.

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_shared_css.html (right):

https://codereview.chromium.org/2202723004/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_shared_css.html:357: height: 28px;
On 2016/08/05 at 00:07:59, Dan Beam wrote:
> what if the font-size changes?  does 28px still work?

It works (at least for Latin characters), see http://imgur.com/a/Pnjou where I
changed fonts to "Very large". Perhaps we could add some top and bottom padding
in the next line to be more robust.

[dpapad, 2016-08-05 18:31:54]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2016-08-05 18:31:55]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2202723004/#ps220001
(title: "Nit.")

[commit-bot: I haz the power, 2016-08-05 18:32:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 19:18:31]

Committed patchset #3 (id:220001)

[commit-bot: I haz the power, 2016-08-05 19:21:09]

Description was changed from

==========
MD Settings: Highlight sub-page search hits.

Specifically:
 - When a search hit occurs in a subpage, display a search bubble
   next to the element that is desginated as the "associated control"
   The search bubble guides the user to find the search hit.
 - Address case where multiple search hits occur in the same supgage,
   by ensuring that only one search bubble is added.
 - Update "unhighlight" codepath to also remove search bubbles.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Highlight sub-page search hits.

Specifically:
 - When a search hit occurs in a subpage, display a search bubble
   next to the element that is desginated as the "associated control"
   The search bubble guides the user to find the search hit.
 - Address case where multiple search hits occur in the same supgage,
   by ensuring that only one search bubble is added.
 - Update "unhighlight" codepath to also remove search bubbles.

BUG=633657
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/52c0e1b022a26550030b711b7e2019a4a0bfa3d4
Cr-Commit-Position: refs/heads/master@{#410136}
==========

[commit-bot: I haz the power, 2016-08-05 19:21:10]

Patchset 3 (id:??) landed as
https://crrev.com/52c0e1b022a26550030b711b7e2019a4a0bfa3d4
Cr-Commit-Position: refs/heads/master@{#410136}