Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1666623006:
MD Settings: Manage search engines 3/3, hooking up UI. (Closed)
|
Created:
4 years, 10 months ago by dpapad Modified:
4 years, 10 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@manage_search_engines_handler Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Manage search engines 3/3, hooking up UI.
Specifically,
- Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers.
- Implementing editing of existing search engine entries (and renamed
add_search_engine_dialog to search_engine_dialog).
- Hooking up validiation when editing/adding a search engine.
- Some UI polish (show search engine icons when available, adjust column widths).
- Adding lot of tests.
BUG=479359
Committed: https://crrev.com/844b1fd74dcf08789cc2b47712d8028c1ee8f1d4
Cr-Commit-Position: refs/heads/master@{#376362}
Patch Set 1 #Patch Set 2 : Implementation. #Patch Set 3 : Nits #Patch Set 4 : Rebasing. #Patch Set 5 : Nits. #
Total comments: 16
Patch Set 6 : Addressing comments. #
Total comments: 38
Patch Set 7 : Addressing comments. #
Total comments: 22
Patch Set 8 : Addressing Tommy's feedback. #
Total comments: 5
Patch Set 9 : undo accidental binding conversion to one-way #
Total comments: 37
Patch Set 10 : Addressing comments. #Patch Set 11 : Addressing more comments. #Patch Set 12 : Refactoring test setup #Patch Set 13 : Remove console.log. #
Total comments: 17
Patch Set 14 : Address nits. #Patch Set 15 : Resolving conflicts with ToT #Dependent Patchsets: Messages
Total messages: 67 (26 generated)
Description was changed from ========== MD Settings: Manage search engines, migrating from private API to handler, WIP. BUG= ========== to ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Hooking up validiation when editing/adding a search engine. BUG=479359 ==========
Description was changed from ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Hooking up validiation when editing/adding a search engine. BUG=479359 ========== to ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries. - Hooking up validiation when editing/adding a search engine. - Some UI polish (show icons when available, adjust column widths). BUG=479359 ==========
Patchset #5 (id:80001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Description was changed from ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries. - Hooking up validiation when editing/adding a search engine. - Some UI polish (show icons when available, adjust column widths). BUG=479359 ========== to ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries. - Hooking up validiation when editing/adding a search engine. - Some UI polish (show icons when available, adjust column widths). - Adding tests. BUG=479359 ==========
Patchset #5 (id:160001) has been deleted
Patchset #4 (id:140001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #2 (id:100001) has been deleted
Description was changed from ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries. - Hooking up validiation when editing/adding a search engine. - Some UI polish (show icons when available, adjust column widths). - Adding tests. BUG=479359 ========== to ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries (and renamed add_search_engine_dialog to search_engine_dialog). - Hooking up validiation when editing/adding a search engine. - Some UI polish (show icons when available, adjust column widths). - Adding tests. BUG=479359 ==========
Patchset #5 (id:240001) has been deleted
Description was changed from ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries (and renamed add_search_engine_dialog to search_engine_dialog). - Hooking up validiation when editing/adding a search engine. - Some UI polish (show icons when available, adjust column widths). - Adding tests. BUG=479359 ========== to ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries (and renamed add_search_engine_dialog to search_engine_dialog). - Hooking up validiation when editing/adding a search engine. - Some UI polish (show icons when available, adjust column widths). - Adding lot of tests. BUG=479359 ==========
Description was changed from ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries (and renamed add_search_engine_dialog to search_engine_dialog). - Hooking up validiation when editing/adding a search engine. - Some UI polish (show icons when available, adjust column widths). - Adding lot of tests. BUG=479359 ========== to ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries (and renamed add_search_engine_dialog to search_engine_dialog). - Hooking up validiation when editing/adding a search engine. - Some UI polish (show search engine icons when available, adjust column widths). - Adding lot of tests. BUG=479359 ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org, tommycli@chromium.org
didn't make it through the test file yet https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:53: // TODO(dpapad): Localize strings nit: . at end, also why not now? https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:57: // If editing an existing search enigne, pre-populate the input fields. enigne -> engine https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.html (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:12: <settings-search-engine-dialog model={{engine}}> ="{{engine}}" https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:18: <iron-icon src={{engine.iconURL}}></iron-icon> src="{{engine.iconURL}}" https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { when this is event listener ever removed? https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:45: webUIListeners_: [], probably makes sense to make a behavior to do stuff like this in the future (similar to EventTracker)
https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { On 2016/02/13 02:03:02, Dan Beam wrote: > when this is event listener ever removed? when is this event listener removed**
https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:53: // TODO(dpapad): Localize strings On 2016/02/13 at 02:03:02, Dan Beam wrote: > nit: . at end, also why not now? Done (added period). Not localizing the strings now to not grow this CL more in terms of files touched. https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:57: // If editing an existing search enigne, pre-populate the input fields. On 2016/02/13 at 02:03:02, Dan Beam wrote: > enigne -> engine Done. https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.html (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:12: <settings-search-engine-dialog model={{engine}}> On 2016/02/13 at 02:03:02, Dan Beam wrote: > ="{{engine}}" Done. https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:18: <iron-icon src={{engine.iconURL}}></iron-icon> On 2016/02/13 at 02:03:02, Dan Beam wrote: > src="{{engine.iconURL}}" Done. https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { On 2016/02/13 at 02:03:29, Dan Beam wrote: > On 2016/02/13 02:03:02, Dan Beam wrote: > > when this is event listener ever removed? > > when is this event listener removed** My understanding is that the dialog itself is being garbage collected once it is removed from the DOM (because of using "restamp" along the dom-if), so there is no need to explicitly remove the listener. This also matches what I am observing by adding a console.log in the listener. If the previous listener was not removed and the dialog was the same, I should be seeing multiple listeners firing as a result of closing the dialog, which is not happening. https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:45: webUIListeners_: [], On 2016/02/13 at 02:03:02, Dan Beam wrote: > probably makes sense to make a behavior to do stuff like this in the future (similar to EventTracker) Agreed, this functionality can be re-used by putting it in a behavior, added a TODO.
https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:53: // TODO(dpapad): Localize strings On 2016/02/16 at 18:48:05, dpapad wrote: > On 2016/02/13 at 02:03:02, Dan Beam wrote: > > nit: . at end, also why not now? > > Done (added period). Not localizing the strings now to not grow this CL more in terms of files touched. BTW, I will also be adding some new string shortly to match latest mocks (for example "Query URL wit h %s"), so I prefer to do all the string related tasks in a separate CL.
https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:53: // TODO(dpapad): Localize strings On 2016/02/16 at 19:23:32, dpapad wrote: > On 2016/02/16 at 18:48:05, dpapad wrote: > > On 2016/02/13 at 02:03:02, Dan Beam wrote: > > > nit: . at end, also why not now? > > > > Done (added period). Not localizing the strings now to not grow this CL more in terms of files touched. > > BTW, I will also be adding some new string shortly to match latest mocks (for example "Query URL wit h %s"), so I prefer to do all the string related tasks in a separate CL. FYI,'sent string changes as a separate CL, https://codereview.chromium.org/1703703002.
On 2016/02/16 at 22:29:04, dpapad wrote: > https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... > File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): > > https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... > chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:53: // TODO(dpapad): Localize strings > On 2016/02/16 at 19:23:32, dpapad wrote: > > On 2016/02/16 at 18:48:05, dpapad wrote: > > > On 2016/02/13 at 02:03:02, Dan Beam wrote: > > > > nit: . at end, also why not now? > > > > > > Done (added period). Not localizing the strings now to not grow this CL more in terms of files touched. > > > > BTW, I will also be adding some new string shortly to match latest mocks (for example "Query URL wit h %s"), so I prefer to do all the string related tasks in a separate CL. > > FYI,'sent string changes as a separate CL, https://codereview.chromium.org/1703703002. PTAL (this is blocking my other LGed CL).
https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/260001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:45: webUIListeners_: [], On 2016/02/16 at 18:48:05, dpapad wrote: > On 2016/02/13 at 02:03:02, Dan Beam wrote: > > probably makes sense to make a behavior to do stuff like this in the future (similar to EventTracker) > > Agreed, this functionality can be re-used by putting it in a behavior, added a TODO. FYI, extracting this as a behavior in follow-up CL (https://codereview.chromium.org/1702063003).
https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:22: on-input="validate_"> ideally these would be on-focus="onSearchEngineFocus_" on-input="onSearchEngineInput_" and they'd /call/ validate_(), but I suppose this is OK https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:102: inputElement.setAttribute('invalid', true); inputElement.valid = isValid; https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:10: /** can you keep these 2 comments joined like they were (even if you remove the example boilerplate)? https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:66: /** @private */ can you document |searchEnginesInfo| and its @type? https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/cr_settings_browsertest.js:82: settings_search_engines_page.registerPageTests(); i don't know that you should be doing all of this in one TEST_F() https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:12: }; this is a copy. can you share this code? https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:22: function defineTestBrowserProxy() { why is this a function just to call it twice and no-op the second+ times rather than just statically initialized? var testBrowserProxyLoaded = PolymerTest.importHtml(testBrowserProxyPath).then( https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:30: * A test version of SearchEnginesBrowserProxy. Provides helper methods for wrap at 80 cols https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:52: buildResolverMap_: function() { put this in ctor https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:164: 'chrome://md-settings/search_engines_page/search_engine_dialog.html'), indent off https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:176: teardown(function() {dialog.remove();}); {\sdialog.remove();\s} https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:214: assertEquals(0, inputElement.value.length); how is this better than: assertEquals('', dialog.$.thing.value); ? https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:228: var actionButton = dialog.$['actionButton']; why is this better than .$.actionButton? https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:228: var actionButton = dialog.$['actionButton']; lower definition right before use https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:317: assertEquals(!visible, buttonEl.hidden); how is this better than assertFalse(entry.$.someId.hidden); assertTrue(entry.$.someId.hidden); https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:326: entry.set('engine', engine); do you need to call Polymer.dom.flush() here? https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:343: assertTrue(!!dialog); what's the point of this? it'll just blow up on the next line
> Specifically, > - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. > - Implementing editing of existing search engine entries (and renamed > add_search_engine_dialog to search_engine_dialog). > - Hooking up validiation when editing/adding a search engine. > - Some UI polish (show search engine icons when available, adjust column widths). > - Adding lot of tests. I spy many CLs wrapped up in 1.
Regarding many CLs being wrapped up in 1: I could extract item "Some UI polish (show search engine icons when available, adjust column widths)." but the rest are not that separate, unless I was OK with temporarily regressing functionality as part of the migration to C++ handlers from chrome.searchEnginesPrivate API. If that is a problem though, it would have been much more helpful if you had mentioned this on Friday when first came across this CL. Since you already reviewed, can we keep it as is? As a bonus, the follow up will be removing chrome.searchEnginesPrivate https://codereview.chromium.org/1688333002 ;> https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:102: inputElement.setAttribute('invalid', true); On 2016/02/17 at 02:21:35, Dan Beam wrote: > inputElement.valid = isValid; If you are suggesting to replace both setAttribute and removeAttribute with this single line, tried this and does not work. https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:66: /** @private */ On 2016/02/17 at 02:21:35, Dan Beam wrote: > can you document |searchEnginesInfo| and its @type? Done. The typedef is already defined (see search_engines_browser_proxy.js), but had forgotten to type the annotation here. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/cr_settings_browsertest.js:82: settings_search_engines_page.registerPageTests(); On 2016/02/17 at 02:21:35, Dan Beam wrote: > i don't know that you should be doing all of this in one TEST_F() Any reasons why not? The tests share a bit of setup (like defining the TestBrowserProxy class), and it would take longer to run if we split them. So far they seem to be doing a good job not interfering with each other, so I prefer to keep one TEST_F for each sub-page (similarly to ResetPage tests above). https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:12: }; On 2016/02/17 at 02:21:36, Dan Beam wrote: > this is a copy. can you share this code? Added a TODO. Prefer not to block this CL on this TODO, but will follow up. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:22: function defineTestBrowserProxy() { On 2016/02/17 at 02:21:36, Dan Beam wrote: > why is this a function just to call it twice and no-op the second+ times rather than just statically initialized? > > var testBrowserProxyLoaded = PolymerTest.importHtml(testBrowserProxyPath).then( Tried this, but PolymerTest.importHTML fails unless you call it from within setUp(), see error. "Uncaught TypeError: Cannot read property 'push' of undefined", source: polymer_browser_test_base.js (136) because the underlying array is only initialized in setUp(). https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:30: * A test version of SearchEnginesBrowserProxy. Provides helper methods for On 2016/02/17 at 02:21:36, Dan Beam wrote: > wrap at 80 cols Done. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:52: buildResolverMap_: function() { On 2016/02/17 at 02:21:36, Dan Beam wrote: > put this in ctor Done. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:164: 'chrome://md-settings/search_engines_page/search_engine_dialog.html'), On 2016/02/17 at 02:21:36, Dan Beam wrote: > indent off Done. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:176: teardown(function() {dialog.remove();}); On 2016/02/17 at 02:21:36, Dan Beam wrote: > {\sdialog.remove();\s} Done. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:214: assertEquals(0, inputElement.value.length); On 2016/02/17 at 02:21:36, Dan Beam wrote: > how is this better than: > > assertEquals('', dialog.$.thing.value); > > ? Not sure of what you are contrasting here. Length check vs empty string check? They seem equivalent to me, not worth the ink of those lines. How is the suggestion any better? https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:228: var actionButton = dialog.$['actionButton']; On 2016/02/17 at 02:21:36, Dan Beam wrote: > why is this better than .$.actionButton? Done. Why better? 1) If we ever compile this, compiler will not complain about former, but will about latter (unless PolymerPass is that good, which will be nice). 2) In my editor the foo.$['bar'] is more readable, because 'bar' stands out in red font VS foo.$.bar which is all white and nothing stands out. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:317: assertEquals(!visible, buttonEl.hidden); On 2016/02/17 at 02:21:36, Dan Beam wrote: > how is this better than > > assertFalse(entry.$.someId.hidden); > assertTrue(entry.$.someId.hidden); Because I don't have to if branch as follows if (visible) assertTrue(entry.$.someId.hidden); else assertFalse(entry.$.someId.hidden); I can just use assertEquals() in one line. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:326: entry.set('engine', engine); On 2016/02/17 at 02:21:35, Dan Beam wrote: > do you need to call Polymer.dom.flush() here? Well, it does not seem that this is needed (guessing because I am using the Polymer helper method "set()"). I prefer not adding calls just to be sure, confusing future readers. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:343: assertTrue(!!dialog); On 2016/02/17 at 02:21:35, Dan Beam wrote: > what's the point of this? it'll just blow up on the next line Because the error message will be clearer if it blows up here. It also more explicit IMO.
dpapad: Hope these comments are helpful. Thanks. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:13: <span class="title">{{dialogTitle_}}</span> nit: [[foo_]] since it's not a two-way binding https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:38: {{actionButtonText_}} nit: And here https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:21: model: Object, It appears that this is basically passed in as a constructor argument and used to populate the other properties. It also appears that you do not want to actually edit this object's properties. In that case, would it make more sense to explicitly pass in the below properties (searchEngine_, keyword_, etc.) with one-way bindings? (probably with an additional addNew boolean so it's explicit instead of implicit?). https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:64: this.actionButtonText_ = loadTimeData.getString('searchEnginesAdd'); Although this method is probably faster, I think it'd be more Polymeric to do this with a computed binding with the "addNew" boolean as the param https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.html (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:12: <settings-search-engine-dialog model="{{engine}}"> Yeah here's where I recommend doing one way bindings with the engine.keyword, etc. subproperties. This kinda makes it look like you're editing the engine object itself. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:18: <iron-icon src="{{engine.iconURL}}"></iron-icon> 1 way bindings right? [[]] https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:20: <div class="name">{{engine.displayName}}</div> same here https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { I think this adds a new listener every time Edit is pressed. I saw the previous comment that this listener is destroyed when the dialog itself is destroyed. Still a bit un-intuitive though. Are all these shenanigans just to stamp and restamp the template? Not sure if it's worth it... maybe it should be encapsulated within search_engine_dialog itself? If this is a common pattern maybe we should invent a dialog-restamper element. Again, I feel like paper-dialog itself should just be smart about the restamping business. It might not be worth it to introduce this complexity here.
Thanks for the comments Tommy. Addressed them below, PTAL. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:13: <span class="title">{{dialogTitle_}}</span> On 2016/02/17 at 19:05:15, tommycli wrote: > nit: [[foo_]] since it's not a two-way binding Done (here and elsewhere). https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:38: {{actionButtonText_}} On 2016/02/17 at 19:05:15, tommycli wrote: > nit: And here Done. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:21: model: Object, On 2016/02/17 at 19:05:15, tommycli wrote: > It appears that this is basically passed in as a constructor argument and used to populate the other properties. It also appears that you do not want to actually edit this object's properties. > > In that case, would it make more sense to explicitly pass in the below properties (searchEngine_, keyword_, etc.) with one-way bindings? (probably with an additional addNew boolean so it's explicit instead of implicit?). Yes, I am using "model" as the equivalent of a constructor parameter. I prefer not to flatten SearchEngine by individually passing its internals, because that would be too verbose (and it would also need to be duplicated in search_engine_entry.js which also uses similar constructor parameter). Also, there are more parameters I am going to use (in follow-up CL) from the SearchEngine typedef, for example "canBeEdited", and if I flatten it here, I would have to keep adding such properties as needed, seems inconvenient. I changed the binding to "[[]]" when I pass the object to the constructor which should clarify any concerns about editing this property directly. From the docs "To avoid two-way binding, use “square-brace” syntax ([[property]]), which results in only one-way (downward, host-to-child) data-binding." https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:64: this.actionButtonText_ = loadTimeData.getString('searchEnginesAdd'); On 2016/02/17 at 19:05:15, tommycli wrote: > Although this method is probably faster, I think it'd be more Polymeric to do this with a computed binding with the "addNew" boolean as the param I tried your suggestion, but using "model" since I am not using "addNew" (per previous comment). Unfortunately computed binding does not trigger when model == null. I could still use it by assigning a default object instead of null, but then the computed binding looks weird getDialogTitle_: function(model) { return Object.keys(model).length == 0 ? 'Add search engine' : 'Edit search engine'; }, And also this breaks the type annotation because empty object is not of type "SearchEngine". How about keeping this as is since that it is already Polymeric enough? dialogTitle_, actionButtonText_ use data binding (just not computed binding). https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.html (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:12: <settings-search-engine-dialog model="{{engine}}"> On 2016/02/17 at 19:05:15, tommycli wrote: > Yeah here's where I recommend doing one way bindings with the engine.keyword, etc. subproperties. This kinda makes it look like you're editing the engine object itself. Done. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:18: <iron-icon src="{{engine.iconURL}}"></iron-icon> On 2016/02/17 at 19:05:15, tommycli wrote: > 1 way bindings right? [[]] Done. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.html:20: <div class="name">{{engine.displayName}}</div> On 2016/02/17 at 19:05:15, tommycli wrote: > same here Done (here and below). https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { On 2016/02/17 at 19:05:15, tommycli wrote: > I think this adds a new listener every time Edit is pressed. I saw the previous comment that this listener is destroyed when the dialog itself is destroyed. Still a bit un-intuitive though. > > Are all these shenanigans just to stamp and restamp the template? Not sure if it's worth it... maybe it should be encapsulated within search_engine_dialog itself? If this is a common pattern maybe we should invent a dialog-restamper element. > > Again, I feel like paper-dialog itself should just be smart about the restamping business. It might not be worth it to introduce this complexity here. You are right that a new listener is added every time "Edit" is pressed, but on a brand-new instance of the dialog. Previous dialog is destroyed along with its listeners. Here is the complication I am trying to avoid. 1) dom-if is bound to a the showEditSearchEngineDialog_ boolean that determines whether the dialog is in the DOM. 2) A dialog once in the DOM, can be hidden or shown. If dialog is in the DOM and hidden, then the dom-if showEditSearchEngineDialog_ boolean is still "true", which seems very unintuitive. So I am trying to always flip that boolean to false when the dialog is hidden, for consistency. I am also leveraging the restamp behavior to discard the contents of the dialog that was just closed. I can't find a way to flip the showEditSearchEngineDialog_ boolean back to false, without adding a listener. If you think there is a solution where I don't have to listen for iron-overly-closed event at all, I would be happy to hear.
dpapad: Some more comments for you below. I think we are getting close. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:21: model: Object, On 2016/02/17 22:12:12, dpapad wrote: > On 2016/02/17 at 19:05:15, tommycli wrote: > > It appears that this is basically passed in as a constructor argument and used > to populate the other properties. It also appears that you do not want to > actually edit this object's properties. > > > > In that case, would it make more sense to explicitly pass in the below > properties (searchEngine_, keyword_, etc.) with one-way bindings? (probably with > an additional addNew boolean so it's explicit instead of implicit?). > > Yes, I am using "model" as the equivalent of a constructor parameter. > > I prefer not to flatten SearchEngine by individually passing its internals, > because that would be too verbose (and it would also need to be duplicated in > search_engine_entry.js which also uses similar constructor parameter). Also, > there are more parameters I am going to use (in follow-up CL) from the > SearchEngine typedef, for example "canBeEdited", and if I flatten it here, I > would have to keep adding such properties as needed, seems inconvenient. > > I changed the binding to "[[]]" when I pass the object to the constructor which > should clarify any concerns about editing this property directly. From the docs > "To avoid two-way binding, use “square-brace” syntax ([[property]]), which > results in only one-way (downward, host-to-child) data-binding." Sounds good. To clarify, eventually there will be a property on model which is directly edited by this control? https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:64: this.actionButtonText_ = loadTimeData.getString('searchEnginesAdd'); On 2016/02/17 22:12:12, dpapad wrote: > On 2016/02/17 at 19:05:15, tommycli wrote: > > Although this method is probably faster, I think it'd be more Polymeric to do > this with a computed binding with the "addNew" boolean as the param > > I tried your suggestion, but using "model" since I am not using "addNew" (per > previous comment). Unfortunately computed binding does not trigger when model == > null. I could still use it by assigning a default object instead of null, but > then the computed binding looks weird > > getDialogTitle_: function(model) { > return Object.keys(model).length == 0 ? > 'Add search engine' : 'Edit search engine'; > }, > And also this breaks the type annotation because empty object is not of type > "SearchEngine". > > How about keeping this as is since that it is already Polymeric enough? > dialogTitle_, actionButtonText_ use data binding (just not computed binding). Fine with me. Thanks for giving it a shot. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { On 2016/02/17 22:12:13, dpapad wrote: > On 2016/02/17 at 19:05:15, tommycli wrote: > > I think this adds a new listener every time Edit is pressed. I saw the > previous comment that this listener is destroyed when the dialog itself is > destroyed. Still a bit un-intuitive though. > > > > Are all these shenanigans just to stamp and restamp the template? Not sure if > it's worth it... maybe it should be encapsulated within search_engine_dialog > itself? If this is a common pattern maybe we should invent a dialog-restamper > element. > > > > Again, I feel like paper-dialog itself should just be smart about the > restamping business. It might not be worth it to introduce this complexity here. > > > You are right that a new listener is added every time "Edit" is pressed, but on > a brand-new instance of the dialog. Previous dialog is destroyed along with its > listeners. > > Here is the complication I am trying to avoid. > 1) dom-if is bound to a the showEditSearchEngineDialog_ boolean that determines > whether the dialog is in the DOM. > 2) A dialog once in the DOM, can be hidden or shown. > > If dialog is in the DOM and hidden, then the dom-if showEditSearchEngineDialog_ > boolean is still "true", which seems very unintuitive. So I am trying to always > flip that boolean to false when the dialog is hidden, for consistency. I am also > leveraging the restamp behavior to discard the contents of the dialog that was > just closed. > > I can't find a way to flip the showEditSearchEngineDialog_ boolean back to > false, without adding a listener. If you think there is a solution where I don't > have to listen for iron-overly-closed event at all, I would be happy to hear. Can the dom-if be encapsulated within the search-engine-dialog element itself? I added some comments to that class. Or I think maybe even better: just remove the dom-if. The whole dialog will be dom-ifed out if the subpage is hidden anyways. https://codereview.chromium.org/1666623006/diff/320001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html (right): https://codereview.chromium.org/1666623006/diff/320001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:10: <paper-dialog modal id="dialog"> potentially: dom-if surrounding this element. Though I think you should probably just drop the dom-if. https://codereview.chromium.org/1666623006/diff/320001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/320001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:76: potentially: make an 'open' method that flags the dom-if to true and opens the dialog. I think it's better to have an explicit open method than relying on dom-if to pop up the dialog anyways. You might have to move some initialization code to that instead of in ready/attached. https://codereview.chromium.org/1666623006/diff/320001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:80: this.$.dialog.close(); unflag the dom-if here https://codereview.chromium.org/1666623006/diff/320001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:87: this.$.dialog.close(); and also unflag the dom-if here.
https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:21: model: Object, On 2016/02/17 at 23:47:47, tommycli wrote: > On 2016/02/17 22:12:12, dpapad wrote: > > On 2016/02/17 at 19:05:15, tommycli wrote: > > > It appears that this is basically passed in as a constructor argument and used > > to populate the other properties. It also appears that you do not want to > > actually edit this object's properties. > > > > > > In that case, would it make more sense to explicitly pass in the below > > properties (searchEngine_, keyword_, etc.) with one-way bindings? (probably with > > an additional addNew boolean so it's explicit instead of implicit?). > > > > Yes, I am using "model" as the equivalent of a constructor parameter. > > > > I prefer not to flatten SearchEngine by individually passing its internals, > > because that would be too verbose (and it would also need to be duplicated in > > search_engine_entry.js which also uses similar constructor parameter). Also, > > there are more parameters I am going to use (in follow-up CL) from the > > SearchEngine typedef, for example "canBeEdited", and if I flatten it here, I > > would have to keep adding such properties as needed, seems inconvenient. > > > > I changed the binding to "[[]]" when I pass the object to the constructor which > > should clarify any concerns about editing this property directly. From the docs > > "To avoid two-way binding, use “square-brace” syntax ([[property]]), which > > results in only one-way (downward, host-to-child) data-binding." > > Sounds good. > > To clarify, eventually there will be a property on model which is directly edited by this control? No, it will not. This component calls methods on browserProxy to edit/add a search engine. When that happens an event is sent from C++ to update the model. The "model" JS variable here is never edited by this component. https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { On 2016/02/17 at 23:47:47, tommycli wrote: > On 2016/02/17 22:12:13, dpapad wrote: > > On 2016/02/17 at 19:05:15, tommycli wrote: > > > I think this adds a new listener every time Edit is pressed. I saw the > > previous comment that this listener is destroyed when the dialog itself is > > destroyed. Still a bit un-intuitive though. > > > > > > Are all these shenanigans just to stamp and restamp the template? Not sure if > > it's worth it... maybe it should be encapsulated within search_engine_dialog > > itself? If this is a common pattern maybe we should invent a dialog-restamper > > element. > > > > > > Again, I feel like paper-dialog itself should just be smart about the > > restamping business. It might not be worth it to introduce this complexity here. > > > > > > You are right that a new listener is added every time "Edit" is pressed, but on > > a brand-new instance of the dialog. Previous dialog is destroyed along with its > > listeners. > > > > Here is the complication I am trying to avoid. > > 1) dom-if is bound to a the showEditSearchEngineDialog_ boolean that determines > > whether the dialog is in the DOM. > > 2) A dialog once in the DOM, can be hidden or shown. > > > > If dialog is in the DOM and hidden, then the dom-if showEditSearchEngineDialog_ > > boolean is still "true", which seems very unintuitive. So I am trying to always > > flip that boolean to false when the dialog is hidden, for consistency. I am also > > leveraging the restamp behavior to discard the contents of the dialog that was > > just closed. > > > > I can't find a way to flip the showEditSearchEngineDialog_ boolean back to > > false, without adding a listener. If you think there is a solution where I don't > > have to listen for iron-overly-closed event at all, I would be happy to hear. > > Can the dom-if be encapsulated within the search-engine-dialog element itself? I added some comments to that class. > > Or I think maybe even better: just remove the dom-if. The whole dialog will be dom-ifed out if the subpage is hidden anyways. The primary (maybe the only) reason for using dom-if is to lazily instantiate the dialog such that we don't have extra Polymer components in the DOM that are hidden/invisible etc. Moving the dom-if there would defeat the purpose, I could then just always have the dialog in the dom, but closed (without using dom-if at all), but this is a big waste (more JS running than necessary, more components on the page). https://codereview.chromium.org/1666623006/diff/320001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/320001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:76: On 2016/02/17 at 23:47:48, tommycli wrote: > potentially: make an 'open' method that flags the dom-if to true and opens the dialog. > > I think it's better to have an explicit open method than relying on dom-if to pop up the dialog anyways. > > You might have to move some initialization code to that instead of in ready/attached. See my previous comment on why I don't think it makes much sense to move the dom-if inside this element.
https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { On 2016/02/18 00:05:35, dpapad wrote: > On 2016/02/17 at 23:47:47, tommycli wrote: > > On 2016/02/17 22:12:13, dpapad wrote: > > > On 2016/02/17 at 19:05:15, tommycli wrote: > > > > I think this adds a new listener every time Edit is pressed. I saw the > > > previous comment that this listener is destroyed when the dialog itself is > > > destroyed. Still a bit un-intuitive though. > > > > > > > > Are all these shenanigans just to stamp and restamp the template? Not sure > if > > > it's worth it... maybe it should be encapsulated within search_engine_dialog > > > itself? If this is a common pattern maybe we should invent a > dialog-restamper > > > element. > > > > > > > > Again, I feel like paper-dialog itself should just be smart about the > > > restamping business. It might not be worth it to introduce this complexity > here. > > > > > > > > > You are right that a new listener is added every time "Edit" is pressed, but > on > > > a brand-new instance of the dialog. Previous dialog is destroyed along with > its > > > listeners. > > > > > > Here is the complication I am trying to avoid. > > > 1) dom-if is bound to a the showEditSearchEngineDialog_ boolean that > determines > > > whether the dialog is in the DOM. > > > 2) A dialog once in the DOM, can be hidden or shown. > > > > > > If dialog is in the DOM and hidden, then the dom-if > showEditSearchEngineDialog_ > > > boolean is still "true", which seems very unintuitive. So I am trying to > always > > > flip that boolean to false when the dialog is hidden, for consistency. I am > also > > > leveraging the restamp behavior to discard the contents of the dialog that > was > > > just closed. > > > > > > I can't find a way to flip the showEditSearchEngineDialog_ boolean back to > > > false, without adding a listener. If you think there is a solution where I > don't > > > have to listen for iron-overly-closed event at all, I would be happy to > hear. > > > > Can the dom-if be encapsulated within the search-engine-dialog element itself? > I added some comments to that class. > > > > Or I think maybe even better: just remove the dom-if. The whole dialog will be > dom-ifed out if the subpage is hidden anyways. > > The primary (maybe the only) reason for using dom-if is to lazily instantiate > the dialog such that we don't have extra Polymer components in the DOM that are > hidden/invisible etc. Moving the dom-if there would defeat the purpose, I could > then just always have the dialog in the dom, but closed (without using dom-if at > all), but this is a big waste (more JS running than necessary, more components > on the page). I acknowledge that the current way is probably the fastest and uses the least memory. If the dom-if is moved, it would make the search-engines-dialog element always present, but the inner paper-dialog element and sub elements would be still lazily instantiated right? Seems like it would be worth it for encapsulation purposes. I would also support just removing the dom-if entirely, for simplicity purposes. I may just be more biased in favor of simplicity/encapsulation > performance.
On 2016/02/18 at 00:14:27, tommycli wrote: > https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... > File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): > > https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... > chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: dialog.addEventListener('iron-overlay-closed', function() { > On 2016/02/18 00:05:35, dpapad wrote: > > On 2016/02/17 at 23:47:47, tommycli wrote: > > > On 2016/02/17 22:12:13, dpapad wrote: > > > > On 2016/02/17 at 19:05:15, tommycli wrote: > > > > > I think this adds a new listener every time Edit is pressed. I saw the > > > > previous comment that this listener is destroyed when the dialog itself is > > > > destroyed. Still a bit un-intuitive though. > > > > > > > > > > Are all these shenanigans just to stamp and restamp the template? Not sure > > if > > > > it's worth it... maybe it should be encapsulated within search_engine_dialog > > > > itself? If this is a common pattern maybe we should invent a > > dialog-restamper > > > > element. > > > > > > > > > > Again, I feel like paper-dialog itself should just be smart about the > > > > restamping business. It might not be worth it to introduce this complexity > > here. > > > > > > > > > > > > You are right that a new listener is added every time "Edit" is pressed, but > > on > > > > a brand-new instance of the dialog. Previous dialog is destroyed along with > > its > > > > listeners. > > > > > > > > Here is the complication I am trying to avoid. > > > > 1) dom-if is bound to a the showEditSearchEngineDialog_ boolean that > > determines > > > > whether the dialog is in the DOM. > > > > 2) A dialog once in the DOM, can be hidden or shown. > > > > > > > > If dialog is in the DOM and hidden, then the dom-if > > showEditSearchEngineDialog_ > > > > boolean is still "true", which seems very unintuitive. So I am trying to > > always > > > > flip that boolean to false when the dialog is hidden, for consistency. I am > > also > > > > leveraging the restamp behavior to discard the contents of the dialog that > > was > > > > just closed. > > > > > > > > I can't find a way to flip the showEditSearchEngineDialog_ boolean back to > > > > false, without adding a listener. If you think there is a solution where I > > don't > > > > have to listen for iron-overly-closed event at all, I would be happy to > > hear. > > > > > > Can the dom-if be encapsulated within the search-engine-dialog element itself? > > I added some comments to that class. > > > > > > Or I think maybe even better: just remove the dom-if. The whole dialog will be > > dom-ifed out if the subpage is hidden anyways. > > > > The primary (maybe the only) reason for using dom-if is to lazily instantiate > > the dialog such that we don't have extra Polymer components in the DOM that are > > hidden/invisible etc. Moving the dom-if there would defeat the purpose, I could > > then just always have the dialog in the dom, but closed (without using dom-if at > > all), but this is a big waste (more JS running than necessary, more components > > on the page). > > I acknowledge that the current way is probably the fastest and uses the least memory. > > If the dom-if is moved, it would make the search-engines-dialog element always present, but the inner paper-dialog element and sub elements would be still lazily instantiated right? > > Seems like it would be worth it for encapsulation purposes. I would also support just removing the dom-if entirely, for simplicity purposes. > > I may just be more biased in favor of simplicity/encapsulation > performance. I agree fully on your assessment on performance, from best to worse: current way -> dom-if paper-dialog only -> remove dom-if completly. But with regards to complexity, It seems that dom-ifing only the paper-dialog within search-engine-dialog will be more complex. Basically, during ready/attached there will be an empty local DOM, and no meaningful task can be performed. Everything currently in ready/attached would have to be moved within some this.async() when the paper-dialog is actually dom-ifed to true. Testing would also become more complex. My assessment of dom-if complexity for the approaches we have mentioned is, from least complex to most complex no dom-if at all -> currenty way -> paper-dialog dom-if only. Current way seems to be a good trade-off between performance and complexity IMO.
On 2016/02/18 00:36:39, dpapad wrote: > On 2016/02/18 at 00:14:27, tommycli wrote: > > > https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... > > File > chrome/browser/resources/settings/search_engines_page/search_engine_entry.js > (right): > > > > > https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... > > > chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: > dialog.addEventListener('iron-overlay-closed', function() { > > On 2016/02/18 00:05:35, dpapad wrote: > > > On 2016/02/17 at 23:47:47, tommycli wrote: > > > > On 2016/02/17 22:12:13, dpapad wrote: > > > > > On 2016/02/17 at 19:05:15, tommycli wrote: > > > > > > I think this adds a new listener every time Edit is pressed. I saw the > > > > > previous comment that this listener is destroyed when the dialog itself > is > > > > > destroyed. Still a bit un-intuitive though. > > > > > > > > > > > > Are all these shenanigans just to stamp and restamp the template? Not > sure > > > if > > > > > it's worth it... maybe it should be encapsulated within > search_engine_dialog > > > > > itself? If this is a common pattern maybe we should invent a > > > dialog-restamper > > > > > element. > > > > > > > > > > > > Again, I feel like paper-dialog itself should just be smart about the > > > > > restamping business. It might not be worth it to introduce this > complexity > > > here. > > > > > > > > > > > > > > > You are right that a new listener is added every time "Edit" is pressed, > but > > > on > > > > > a brand-new instance of the dialog. Previous dialog is destroyed along > with > > > its > > > > > listeners. > > > > > > > > > > Here is the complication I am trying to avoid. > > > > > 1) dom-if is bound to a the showEditSearchEngineDialog_ boolean that > > > determines > > > > > whether the dialog is in the DOM. > > > > > 2) A dialog once in the DOM, can be hidden or shown. > > > > > > > > > > If dialog is in the DOM and hidden, then the dom-if > > > showEditSearchEngineDialog_ > > > > > boolean is still "true", which seems very unintuitive. So I am trying to > > > always > > > > > flip that boolean to false when the dialog is hidden, for consistency. I > am > > > also > > > > > leveraging the restamp behavior to discard the contents of the dialog > that > > > was > > > > > just closed. > > > > > > > > > > I can't find a way to flip the showEditSearchEngineDialog_ boolean back > to > > > > > false, without adding a listener. If you think there is a solution where > I > > > don't > > > > > have to listen for iron-overly-closed event at all, I would be happy to > > > hear. > > > > > > > > Can the dom-if be encapsulated within the search-engine-dialog element > itself? > > > I added some comments to that class. > > > > > > > > Or I think maybe even better: just remove the dom-if. The whole dialog > will be > > > dom-ifed out if the subpage is hidden anyways. > > > > > > The primary (maybe the only) reason for using dom-if is to lazily > instantiate > > > the dialog such that we don't have extra Polymer components in the DOM that > are > > > hidden/invisible etc. Moving the dom-if there would defeat the purpose, I > could > > > then just always have the dialog in the dom, but closed (without using > dom-if at > > > all), but this is a big waste (more JS running than necessary, more > components > > > on the page). > > > > I acknowledge that the current way is probably the fastest and uses the least > memory. > > > > If the dom-if is moved, it would make the search-engines-dialog element always > present, but the inner paper-dialog element and sub elements would be still > lazily instantiated right? > > > > Seems like it would be worth it for encapsulation purposes. I would also > support just removing the dom-if entirely, for simplicity purposes. > > > > I may just be more biased in favor of simplicity/encapsulation > performance. > > I agree fully on your assessment on performance, from best to worse: current way > -> dom-if paper-dialog only -> remove dom-if completly. > > But with regards to complexity, It seems that dom-ifing only the paper-dialog > within search-engine-dialog will be more complex. Basically, > during ready/attached there will be an empty local DOM, and no meaningful task > can be performed. Everything currently in ready/attached would > have to be moved within some this.async() when the paper-dialog is actually > dom-ifed to true. Testing would also become more complex. > > My assessment of dom-if complexity for the approaches we have mentioned is, from > least complex to most complex > no dom-if at all -> currenty way -> paper-dialog dom-if only. Current way seems > to be a good trade-off between performance and complexity IMO. Okay. I still have reservations about the current approach because it seems complex and I'm not a big fan of the dom-if to open a dialog. But if you have explored these alternatives thoroughly, I am not going to hold up this CL. lgtm with above reservations.
On 2016/02/18 at 00:39:56, tommycli wrote: > On 2016/02/18 00:36:39, dpapad wrote: > > On 2016/02/18 at 00:14:27, tommycli wrote: > > > > > https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... > > > File > > chrome/browser/resources/settings/search_engines_page/search_engine_entry.js > > (right): > > > > > > > > https://codereview.chromium.org/1666623006/diff/300001/chrome/browser/resourc... > > > > > chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:45: > > dialog.addEventListener('iron-overlay-closed', function() { > > > On 2016/02/18 00:05:35, dpapad wrote: > > > > On 2016/02/17 at 23:47:47, tommycli wrote: > > > > > On 2016/02/17 22:12:13, dpapad wrote: > > > > > > On 2016/02/17 at 19:05:15, tommycli wrote: > > > > > > > I think this adds a new listener every time Edit is pressed. I saw the > > > > > > previous comment that this listener is destroyed when the dialog itself > > is > > > > > > destroyed. Still a bit un-intuitive though. > > > > > > > > > > > > > > Are all these shenanigans just to stamp and restamp the template? Not > > sure > > > > if > > > > > > it's worth it... maybe it should be encapsulated within > > search_engine_dialog > > > > > > itself? If this is a common pattern maybe we should invent a > > > > dialog-restamper > > > > > > element. > > > > > > > > > > > > > > Again, I feel like paper-dialog itself should just be smart about the > > > > > > restamping business. It might not be worth it to introduce this > > complexity > > > > here. > > > > > > > > > > > > > > > > > > You are right that a new listener is added every time "Edit" is pressed, > > but > > > > on > > > > > > a brand-new instance of the dialog. Previous dialog is destroyed along > > with > > > > its > > > > > > listeners. > > > > > > > > > > > > Here is the complication I am trying to avoid. > > > > > > 1) dom-if is bound to a the showEditSearchEngineDialog_ boolean that > > > > determines > > > > > > whether the dialog is in the DOM. > > > > > > 2) A dialog once in the DOM, can be hidden or shown. > > > > > > > > > > > > If dialog is in the DOM and hidden, then the dom-if > > > > showEditSearchEngineDialog_ > > > > > > boolean is still "true", which seems very unintuitive. So I am trying to > > > > always > > > > > > flip that boolean to false when the dialog is hidden, for consistency. I > > am > > > > also > > > > > > leveraging the restamp behavior to discard the contents of the dialog > > that > > > > was > > > > > > just closed. > > > > > > > > > > > > I can't find a way to flip the showEditSearchEngineDialog_ boolean back > > to > > > > > > false, without adding a listener. If you think there is a solution where > > I > > > > don't > > > > > > have to listen for iron-overly-closed event at all, I would be happy to > > > > hear. > > > > > > > > > > Can the dom-if be encapsulated within the search-engine-dialog element > > itself? > > > > I added some comments to that class. > > > > > > > > > > Or I think maybe even better: just remove the dom-if. The whole dialog > > will be > > > > dom-ifed out if the subpage is hidden anyways. > > > > > > > > The primary (maybe the only) reason for using dom-if is to lazily > > instantiate > > > > the dialog such that we don't have extra Polymer components in the DOM that > > are > > > > hidden/invisible etc. Moving the dom-if there would defeat the purpose, I > > could > > > > then just always have the dialog in the dom, but closed (without using > > dom-if at > > > > all), but this is a big waste (more JS running than necessary, more > > components > > > > on the page). > > > > > > I acknowledge that the current way is probably the fastest and uses the least > > memory. > > > > > > If the dom-if is moved, it would make the search-engines-dialog element always > > present, but the inner paper-dialog element and sub elements would be still > > lazily instantiated right? > > > > > > Seems like it would be worth it for encapsulation purposes. I would also > > support just removing the dom-if entirely, for simplicity purposes. > > > > > > I may just be more biased in favor of simplicity/encapsulation > performance. > > > > I agree fully on your assessment on performance, from best to worse: current way > > -> dom-if paper-dialog only -> remove dom-if completly. > > > > But with regards to complexity, It seems that dom-ifing only the paper-dialog > > within search-engine-dialog will be more complex. Basically, > > during ready/attached there will be an empty local DOM, and no meaningful task > > can be performed. Everything currently in ready/attached would > > have to be moved within some this.async() when the paper-dialog is actually > > dom-ifed to true. Testing would also become more complex. > > > > My assessment of dom-if complexity for the approaches we have mentioned is, from > > least complex to most complex > > no dom-if at all -> currenty way -> paper-dialog dom-if only. Current way seems > > to be a good trade-off between performance and complexity IMO. > > Okay. I still have reservations about the current approach because it seems complex and I'm not a big fan of the dom-if to open a dialog. > > But if you have explored these alternatives thoroughly, I am not going to hold up this CL. > > lgtm with above reservations. Thanks. I'll keep your reservations in mind, and see where we're at once Michael's work of lazi-fying instantiation of animated pages is done. Maybe then we can remove dom-ifs completely from animated pages. In the meantime complexity is at least limited at a single place in the code (set boolean to true, add dialog close listener, flip boolean back to false).
https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:10: /** On 2016/02/17 02:21:35, Dan Beam wrote: > can you keep these 2 comments joined like they were (even if you remove the > example boilerplate)? ping https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:69: attached: function() { can this be done in ready() instead? so the tests don't have to add things to the DOM? https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:102: inputElement.setAttribute('invalid', true); https://github.com/PolymerElements/paper-input/blob/master/paper-input-behavi... is autoValidate is on? https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:112: return !inputElement.invalid && inputElement.value.length != 0; you're using .invalid here so I know it exists, you're just having problems setting it https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:19: /** @type {boolean} */ @type -> @private https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:56: /** @private */ https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:30: /** @type {boolean} */ @type -> @private https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:44: * such that it can be re-used. ^ this sounds cool! https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:56: this.enginesChanged_.bind(this))); nit: this can be less lines https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/cr_settings_browsertest.js:39: 'search_engines_page_test.js', please derive CrSettingsBrowserTest instead of doing this, see https://codereview.chromium.org/1700273003/diff/80001/chrome/test/data/webui/... https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:31: 'chrome://md-settings/search_engines_page/search_engines_browser_proxy.html'; why doesn't the page that's visited via browsePreload have <link rel="import" href="chrome://md-settings/search_engines_page/search_engines_browser_proxy.html"> to remove the need for this awkward code? https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:75: * @return {!Promise} A promise that is resolved when the given method is 80 col wrap https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:119: setGetSearchEnginesList: function(searchEnginesInfo) { setSearchEnginesInfo https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:139: var getSampleSearchEngine = function() { get -> create https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:155: \n\n -> \n https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:177: document.body.appendChild(dialog); is appending to the body required? what if we moved to ready() instead of attached()? https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:227: var assertInputValidation = function(inputId) { can this be named focusAndValidate()? https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:421: var addSearchEngineButton = page.$['addSearchEngine']; $.addSearchEngine
Patchset #10 (id:360001) has been deleted
https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:10: /** On 2016/02/18 at 03:08:18, Dan Beam wrote: > On 2016/02/17 02:21:35, Dan Beam wrote: > > can you keep these 2 comments joined like they were (even if you remove the > > example boilerplate)? > > ping Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:69: attached: function() { On 2016/02/18 at 03:08:18, Dan Beam wrote: > can this be done in ready() instead? so the tests don't have to add things to the DOM? See detailed response about this in the test file. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:102: inputElement.setAttribute('invalid', true); On 2016/02/18 at 03:08:18, Dan Beam wrote: > https://github.com/PolymerElements/paper-input/blob/master/paper-input-behavi... > > is autoValidate is on? Done. Tried again, this time the correct way and it works. The problem was that when I tried before I used your suggestion verbatim, "inputElement.valid = isValid;" which is not correct. Regarding autoValidate, it is unrelated, but not using it, because of https://github.com/PolymerElements/iron-validator-behavior/issues/17, autoValidate is not helpful in our case. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:112: return !inputElement.invalid && inputElement.value.length != 0; On 2016/02/18 at 03:08:18, Dan Beam wrote: > you're using .invalid here so I know it exists, you're just having problems setting it See previous comment. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_entry.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:19: /** @type {boolean} */ On 2016/02/18 at 03:08:18, Dan Beam wrote: > @type -> @private Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_entry.js:56: On 2016/02/18 at 03:08:18, Dan Beam wrote: > /** @private */ Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engines_page.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:30: /** @type {boolean} */ On 2016/02/18 at 03:08:19, Dan Beam wrote: > @type -> @private Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:44: * such that it can be re-used. On 2016/02/18 at 03:08:19, Dan Beam wrote: > ^ this sounds cool! https://codereview.chromium.org/1702063003, will send it out after this CL lands. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engines_page.js:56: this.enginesChanged_.bind(this))); On 2016/02/18 at 03:08:19, Dan Beam wrote: > nit: this can be less lines Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/cr_settings_browsertest.js:39: 'search_engines_page_test.js', On 2016/02/18 at 03:08:19, Dan Beam wrote: > please derive CrSettingsBrowserTest instead of doing this, see > https://codereview.chromium.org/1700273003/diff/80001/chrome/test/data/webui/... Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:31: 'chrome://md-settings/search_engines_page/search_engines_browser_proxy.html'; On 2016/02/18 at 03:08:19, Dan Beam wrote: > why doesn't the page that's visited via browsePreload have > > <link rel="import" href="chrome://md-settings/search_engines_page/search_engines_browser_proxy.html"> > > to remove the need for this awkward code? For many reasons, see also reply at line 177. I am unit-testing individual elements, and I need time to setup the test browser proxy before the page is loaded, so browsePreload URL can't be chrome://md-settings/searchEngine because that would not give me time to setup. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:75: * @return {!Promise} A promise that is resolved when the given method is On 2016/02/18 at 03:08:19, Dan Beam wrote: > 80 col wrap Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:119: setGetSearchEnginesList: function(searchEnginesInfo) { On 2016/02/18 at 03:08:19, Dan Beam wrote: > setSearchEnginesInfo Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:139: var getSampleSearchEngine = function() { On 2016/02/18 at 03:08:19, Dan Beam wrote: > get -> create Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:155: On 2016/02/18 at 03:08:19, Dan Beam wrote: > \n\n -> \n Done. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:177: document.body.appendChild(dialog); On 2016/02/18 at 03:08:19, Dan Beam wrote: > is appending to the body required? what if we moved to ready() instead of attached()? There are other elements that the "settings-search-engine-dialog" is using internally that do work on attached (for example https://github.com/PolymerElements/paper-input/blob/master/paper-input-behavi...), so doing this would make this test very fragile (even if it passes for now, which I did not try). I don't see a good reason to test the element while detached, when in the prod code this element is attached to the DOM. The testing methodology I am using in this file is very simple. - Start with the default page of CrSettingsBrowserTest (prefs.html). - Unit test an element by individually loading it (for example search-engine-dialog, search-engine-entry, search-engine-page) and then doing assertions. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:227: var assertInputValidation = function(inputId) { On 2016/02/18 at 03:08:19, Dan Beam wrote: > can this be named focusAndValidate()? Done, but IMO "assert" prefix was making it clearer that this method is checking for something, now it is not that clear that is part of the assertions this test is making. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:421: var addSearchEngineButton = page.$['addSearchEngine']; On 2016/02/18 at 03:08:19, Dan Beam wrote: > $.addSearchEngine Done, but never got a reply on why this is better. See my case at https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui....
https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:228: var actionButton = dialog.$['actionButton']; On 2016/02/17 03:22:36, dpapad wrote: > On 2016/02/17 at 02:21:36, Dan Beam wrote: > > why is this better than .$.actionButton? > > Done. > > Why better? > 1) If we ever compile this, compiler will not complain about former, but will > about latter (unless PolymerPass is that good, which will be nice). polymer pass does have a renaming map, using ['id'] instead of .id would probably be more hurtful than helpful (surprisingly) > 2) In my editor the foo.$['bar'] is more readable, because 'bar' stands out in > red font VS foo.$.bar which is all white and nothing stands out. ['bar'] is actually creating a string whereas .bar is a property accessor https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:317: assertEquals(!visible, buttonEl.hidden); On 2016/02/17 03:22:36, dpapad wrote: > On 2016/02/17 at 02:21:36, Dan Beam wrote: > > how is this better than > > > > assertFalse(entry.$.someId.hidden); > > assertTrue(entry.$.someId.hidden); > > Because I don't have to if branch as follows > if (visible) > assertTrue(entry.$.someId.hidden); > else > assertFalse(entry.$.someId.hidden); > > I can just use assertEquals() in one line. no, I mean assertFalse(entry.$.makeDefault.hidden); assertFalse(entry.$.edit.hidden); assertFalse(entry.$.delete.hidden); // Mark the engine as the "default" one. var engine = getSampleSearchEngine(); engine.default = true; entry.set('engine', engine); assertTrue(entry.$.makeDefault.hidden); assertTrue(entry.$.edit.hidden); assertTrue(entry.$.delete.hidden); https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:326: entry.set('engine', engine); On 2016/02/17 03:22:36, dpapad wrote: > On 2016/02/17 at 02:21:35, Dan Beam wrote: > > do you need to call Polymer.dom.flush() here? > > Well, it does not seem that this is needed (guessing because I am using the > Polymer helper method "set()"). I prefer not adding calls just to be sure, > confusing future readers. your template binds hidden="[[entry.default]]" which might be updated asynchronously. Polymer.dom.flush() forces templates to re-render right away. https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:102: inputElement.setAttribute('invalid', true); On 2016/02/18 19:34:51, dpapad wrote: > On 2016/02/18 at 03:08:18, Dan Beam wrote: > > > https://github.com/PolymerElements/paper-input/blob/master/paper-input-behavi... > > > > is autoValidate is on? > > Done. Tried again, this time the correct way and it works. The problem was that > when I tried before I used your suggestion verbatim, "inputElement.valid = > isValid;" which is not correct. note: codereview.chromium.org does not actually run code, so I wouldn't trust any other reviewer code suggestion blindly ;) > > Regarding autoValidate, it is unrelated, but not using it, because of > https://github.com/PolymerElements/iron-validator-behavior/issues/17, > autoValidate is not helpful in our case. https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:177: document.body.appendChild(dialog); On 2016/02/18 19:34:51, dpapad wrote: > On 2016/02/18 at 03:08:19, Dan Beam wrote: > > is appending to the body required? what if we moved to ready() instead of > attached()? > > There are other elements that the "settings-search-engine-dialog" is using > internally that do work on attached (for example > https://github.com/PolymerElements/paper-input/blob/master/paper-input-behavi...), > so doing this would make this test very fragile (even if it passes for now, > which I did not try). I don't see a good reason to test the element while > detached, when in the prod code this element is attached to the DOM. that's fine if other elements rely on being attached > > The testing methodology I am using in this file is very simple. > - Start with the default page of CrSettingsBrowserTest (prefs.html). we would ideally be loading a specific component's .html file (i.e. search_engine_entry.html) which would have the proper dependencies > - Unit test an element by individually loading it (for example > search-engine-dialog, search-engine-entry, search-engine-page) and then doing > assertions. right
https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:177: document.body.appendChild(dialog); On 2016/02/18 20:32:28, Dan Beam wrote: > On 2016/02/18 19:34:51, dpapad wrote: > > On 2016/02/18 at 03:08:19, Dan Beam wrote: > > > is appending to the body required? what if we moved to ready() instead of > > attached()? > > > > There are other elements that the "settings-search-engine-dialog" is using > > internally that do work on attached (for example > > > https://github.com/PolymerElements/paper-input/blob/master/paper-input-behavi...), > > so doing this would make this test very fragile (even if it passes for now, > > which I did not try). I don't see a good reason to test the element while > > detached, when in the prod code this element is attached to the DOM. > > that's fine if other elements rely on being attached > > > > > The testing methodology I am using in this file is very simple. > > - Start with the default page of CrSettingsBrowserTest (prefs.html). > > we would ideally be loading a specific component's .html file (i.e. > search_engine_entry.html) which would have the proper dependencies here's an example of this: https://codereview.chromium.org/1700273003/diff/80001/chrome/test/data/webui/... https://codereview.chromium.org/1700273003/diff/80001/chrome/test/data/webui/... > > > - Unit test an element by individually loading it (for example > > search-engine-dialog, search-engine-entry, search-engine-page) and then doing > > assertions. > > right
did I mention already that author+review lives might be easier if you consciously decided to do things in smaller steps before you started working on this/posted this review?
On 2016/02/18 at 20:40:32, dbeam wrote: > did I mention already that author+review lives might be easier if you consciously decided to do things in smaller steps before you started working on this/posted this review? I tried to break the overall task to 3 CLs (see 1/3, 2/3, 3/3 in the CL description). I thought that was a good enough trade-off of not doing monster CLs vs too many tiny CLs. Apparently this review created a lot of friction which I did not expect, and had I known this I would split it even further (and would hope that review turn-around times would be faster, which was also a factor in my decision to not split more). I hope this gives you some context for my decision making process on how much to split.
On 2016/02/18 20:43:27, dpapad wrote: > On 2016/02/18 at 20:40:32, dbeam wrote: > > did I mention already that author+review lives might be easier if you > consciously decided to do things in smaller steps before you started working on > this/posted this review? > > I tried to break the overall task to 3 CLs (see 1/3, 2/3, 3/3 in the CL > description). I thought that was a good enough trade-off of not doing monster > CLs vs too many tiny CLs. Apparently this review created a lot of friction which > I did not expect, and had I known this I would split it even further (and would > hope that review turn-around times would be faster, which was also a factor in > my decision to not split more). I hope this gives you some context for my > decision making process on how much to split. every time a review is large, and the entire thing gets held up on small things, there will be friction. the reviewer feels bad because they have to hold up a ton of extra good code. but it doesn't really matter because code is code: if it was a small review we wouldn't overlook the small details. authors are frustrated that 95% of the code is ready to go but that 5% is blocking everything. also more merging.
I'm not necessarily suggesting splitting this review at this point (unless you'd like to land the things I don't have an object to, which are really only your tests right now) I'm recommending thinking about how to split things more aggressively before posting the next patch is all.
an objection to*
On 2016/02/18 at 20:37:56, dbeam wrote: > https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... > File chrome/test/data/webui/settings/search_engines_page_test.js (right): > > https://codereview.chromium.org/1666623006/diff/340001/chrome/test/data/webui... > chrome/test/data/webui/settings/search_engines_page_test.js:177: document.body.appendChild(dialog); > On 2016/02/18 20:32:28, Dan Beam wrote: > > On 2016/02/18 19:34:51, dpapad wrote: > > > On 2016/02/18 at 03:08:19, Dan Beam wrote: > > > > is appending to the body required? what if we moved to ready() instead of > > > attached()? > > > > > > There are other elements that the "settings-search-engine-dialog" is using > > > internally that do work on attached (for example > > > > > https://github.com/PolymerElements/paper-input/blob/master/paper-input-behavi...), > > > so doing this would make this test very fragile (even if it passes for now, > > > which I did not try). I don't see a good reason to test the element while > > > detached, when in the prod code this element is attached to the DOM. > > > > that's fine if other elements rely on being attached > > > > > > > > The testing methodology I am using in this file is very simple. > > > - Start with the default page of CrSettingsBrowserTest (prefs.html). > > > > we would ideally be loading a specific component's .html file (i.e. > > search_engine_entry.html) which would have the proper dependencies > > here's an example of this: > https://codereview.chromium.org/1700273003/diff/80001/chrome/test/data/webui/... > https://codereview.chromium.org/1700273003/diff/80001/chrome/test/data/webui/... > > > > > - Unit test an element by individually loading it (for example > > > search-engine-dialog, search-engine-entry, search-engine-page) and then doing > > > assertions. > > > > right Understood. The caveat of this approach is that it requires a 1:1 association of CppTestFixture with the Polymer element being tested, which is a bit verbose compared to one CppTestFixture for many elements.
PTAL https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:317: assertEquals(!visible, buttonEl.hidden); On 2016/02/18 at 20:32:28, Dan Beam wrote: > On 2016/02/17 03:22:36, dpapad wrote: > > On 2016/02/17 at 02:21:36, Dan Beam wrote: > > > how is this better than > > > > > > assertFalse(entry.$.someId.hidden); > > > assertTrue(entry.$.someId.hidden); > > > > Because I don't have to if branch as follows > > if (visible) > > assertTrue(entry.$.someId.hidden); > > else > > assertFalse(entry.$.someId.hidden); > > > > I can just use assertEquals() in one line. > > no, I mean > > assertFalse(entry.$.makeDefault.hidden); > assertFalse(entry.$.edit.hidden); > assertFalse(entry.$.delete.hidden); > > // Mark the engine as the "default" one. > var engine = getSampleSearchEngine(); > engine.default = true; > entry.set('engine', engine); > > assertTrue(entry.$.makeDefault.hidden); > assertTrue(entry.$.edit.hidden); > assertTrue(entry.$.delete.hidden); Done. https://codereview.chromium.org/1666623006/diff/280001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:326: entry.set('engine', engine); On 2016/02/18 at 20:32:28, Dan Beam wrote: > On 2016/02/17 03:22:36, dpapad wrote: > > On 2016/02/17 at 02:21:35, Dan Beam wrote: > > > do you need to call Polymer.dom.flush() here? > > > > Well, it does not seem that this is needed (guessing because I am using the > > Polymer helper method "set()"). I prefer not adding calls just to be sure, > > confusing future readers. > > your template binds hidden="[[entry.default]]" which might be updated asynchronously. Polymer.dom.flush() forces templates to re-render right away. Added a call to Polymer.dom.flush().
Patchset #12 (id:420001) has been deleted
Updated per offline discussion. - Test proxy no longer inherits real one, instead implements an interface. - CrSettingsSearchEngine C++ test class loads search_engines_page.html. - No funky async code needed for definition of the test browser proxy. - Moved getInstance() calls to created instead of ready/attached lifecycle methods. PTAL.
lgtm https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:17: <paper-icon-button icon="clear" on-tap="onCancelTap_" id="close"> nit: maybe make a cancel_ or close_ if both #cancel and #close re-use this (i.e. onCancelTap_ makes less sense when id="close", it would be onCloseTap_ in our current guidance) https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:40: browserProxy_: null, i was wondering if we [should] do this i assume you're putting this outside of properties because it's never used in bindings or computed/observers? https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/cr_settings_browsertest.js:78: nit: remove \n https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:38: wrapperMethods.forEach(this.resetResolver.bind(this)); nit: wrapperMethods.forEach(this.resetResolver, this); https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:41: this.searchEnginesInfo_ = {}; i'd say this should be /** @private {?SearchEnginesInfo} */ this.searchEnginesInfo_ = null; or /** @private {!SearchEnginesInfo} */ this.searchEnginesInfo_ = {defaults: [], others: [], extensions: []}; https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:98: return Promise.resolve(this.searchEnginesInfo_); if you end up making this.searchEnginesInfo_ = null, should this be return Promise.resolve(assert(this.searchEnginesInfo_)); ? https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:183: * focused. nit: update description now https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:193: assertInputEmpty('searchEngine'); nit: assertEquals('', dialog.$.searchEngine.value); is better IMO because it tells you the differing content rather than just the length if there's a failure
Thank you. https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html:17: <paper-icon-button icon="clear" on-tap="onCancelTap_" id="close"> On 2016/02/18 at 23:27:41, Dan Beam wrote: > nit: maybe make a cancel_ or close_ if both #cancel and #close re-use this (i.e. onCancelTap_ makes less sense when id="close", it would be onCloseTap_ in our current guidance) Done. https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:40: browserProxy_: null, On 2016/02/18 at 23:27:41, Dan Beam wrote: > i was wondering if we [should] do this > > i assume you're putting this outside of properties because it's never used in bindings or computed/observers? Yes, that is the reason. On top of that I actually tried making this a proper Polymer property and inject it the same way I am injecting SearchEngine objects (see "model" property a few lines above), instead of doing the getInstance() thing, but to my surprise I found out that Polymer properties that are involved in bindings, can't be arbitrary class instances (with prototypes), they can only be Array, Object, and primitives it seems. https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/settings/cr_settings_browsertest.js (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/cr_settings_browsertest.js:78: On 2016/02/18 at 23:27:41, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... File chrome/test/data/webui/settings/search_engines_page_test.js (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:38: wrapperMethods.forEach(this.resetResolver.bind(this)); On 2016/02/18 at 23:27:41, Dan Beam wrote: > nit: wrapperMethods.forEach(this.resetResolver, this); Done. https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:41: this.searchEnginesInfo_ = {}; On 2016/02/18 at 23:27:41, Dan Beam wrote: > i'd say this should be > > /** @private {?SearchEnginesInfo} */ > this.searchEnginesInfo_ = null; > > or > > /** @private {!SearchEnginesInfo} */ > this.searchEnginesInfo_ = {defaults: [], others: [], extensions: []}; Done. https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:98: return Promise.resolve(this.searchEnginesInfo_); On 2016/02/18 at 23:27:41, Dan Beam wrote: > if you end up making this.searchEnginesInfo_ = null, should this be > > return Promise.resolve(assert(this.searchEnginesInfo_)); > > ? I went with the non-null approach. https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:183: * focused. On 2016/02/18 at 23:27:41, Dan Beam wrote: > nit: update description now Done. https://codereview.chromium.org/1666623006/diff/460001/chrome/test/data/webui... chrome/test/data/webui/settings/search_engines_page_test.js:193: assertInputEmpty('searchEngine'); On 2016/02/18 at 23:27:41, Dan Beam wrote: > nit: > > assertEquals('', dialog.$.searchEngine.value); > > is better IMO because it tells you the differing content rather than just the length if there's a failure Done.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1666623006/#ps480001 (title: "Address nits.")
lgtm https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js (right): https://codereview.chromium.org/1666623006/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js:40: browserProxy_: null, On 2016/02/18 23:45:46, dpapad wrote: > On 2016/02/18 at 23:27:41, Dan Beam wrote: > > i was wondering if we [should] do this > > > > i assume you're putting this outside of properties because it's never used in > bindings or computed/observers? > > Yes, that is the reason. > > On top of that I actually tried making this a proper Polymer property and inject > it the same way I am injecting SearchEngine objects (see "model" property a few > lines above), instead of doing the getInstance() thing, but to my surprise I > found out that Polymer properties that are involved in bindings, can't be > arbitrary class instances (with prototypes), they can only be Array, Object, and > primitives it seems. I think type: Object would work /** @private {!settings.SearchEnginesBrowserProxy} */ browserProxy_: { type: Object, value: null, },
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666623006/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666623006/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666623006/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666623006/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...) android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommycli@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1666623006/#ps500001 (title: "Resolving conflicts with ToT")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1666623006/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1666623006/500001
Message was sent while issue was closed.
Committed patchset #15 (id:500001)
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries (and renamed add_search_engine_dialog to search_engine_dialog). - Hooking up validiation when editing/adding a search engine. - Some UI polish (show search engine icons when available, adjust column widths). - Adding lot of tests. BUG=479359 ========== to ========== MD Settings: Manage search engines 3/3, hooking up UI. Specifically, - Migrating UI from chrome.searchEnginesPrivate API to use C++ handlers. - Implementing editing of existing search engine entries (and renamed add_search_engine_dialog to search_engine_dialog). - Hooking up validiation when editing/adding a search engine. - Some UI polish (show search engine icons when available, adjust column widths). - Adding lot of tests. BUG=479359 Committed: https://crrev.com/844b1fd74dcf08789cc2b47712d8028c1ee8f1d4 Cr-Commit-Position: refs/heads/master@{#376362} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/844b1fd74dcf08789cc2b47712d8028c1ee8f1d4 Cr-Commit-Position: refs/heads/master@{#376362} |
