Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13375003:
Fixing iframe jank in the local omnibox popup. (Closed)
|
Created:
7 years, 8 months ago by Jered Modified:
7 years, 8 months ago CC:
chromium-reviews, melevin, dhollowa+watch_chromium.org, dougw+watch_chromium.org, gideonwald, sreeram, dominich, Aaron Boodman, David Black, arv+watch_chromium.org, samarth+watch_chromium.org, chromium-apps-reviews_chromium.org, kmadhusu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixing iframe jank in the local omnibox popup.
1) Previously we created and loaded a new iframe for each suggestion.
Suggestion data was served from the renderer by rewriting
chrome-search://suggestion to data: URLs. This turned out not to be
performant and suffered from other issues like showing a load spinner.
Instead, this change creates one set of iframes with the chrome-search
origin and uses a controlling iframe to coordinate updating their DOMs
with new suggestions. To preserve the chrome-search origin for all the
communicating iframes, it no longer rewrites them as data: URLs and
instead serves data from a URLDataSource in the browser.
2) For flicker free rendering, old suggestion iframes must be hidden in
the same JS context as new ones are shown. Since postMessage() is async,
this necessarily means we must render one set of suggestions while
showing another (i.e. double buffer). This change modifies the local
omnibox popup JS to do that.
3) Several styling and rendering issues are resolved, such as hover,
resize and overflow behavior and positioning. I only tested LTR and not
RTL, so this change may not address any LTR issues.
TEST=Open an incognito window and start typing. Click on suggestions,
arrow up/down, hover, and generally try to break things.
BUG=223608
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196184
Patch Set 1 #
Total comments: 5
Patch Set 2 : Include missing file. #
Total comments: 28
Patch Set 3 : Rebase. #Patch Set 4 : Addressing js comments + fixing display issues. #Patch Set 5 : Addressing cc comments. #
Total comments: 146
Patch Set 6 : Addressing most comments. #Patch Set 7 : Fixing up/down. #
Total comments: 4
Patch Set 8 : Rewording comment. #Patch Set 9 : Fixing default colors. #Patch Set 10 : Rebase. #
Total comments: 41
Patch Set 11 : %s/\$\$/$qs/g #Patch Set 12 : Changing JS style. #
Total comments: 26
Patch Set 13 : Addressing comments. #Patch Set 14 : Asserting. #Patch Set 15 : Adding unit tests. #
Total comments: 18
Patch Set 16 : Addressing comments. #
Total comments: 16
Patch Set 17 : Returning instead of using out param. #Patch Set 18 : Rebase. #Patch Set 19 : Converting to ResourceBundle API. #Patch Set 20 : Addressing comments. #
Total comments: 10
Patch Set 21 : Addressing comments. #
Total comments: 2
Patch Set 22 : Adding assert.js #Patch Set 23 : Merging 13898003. #Patch Set 24 : Setting origin from browser state. #Patch Set 25 : Removing sites, too. #
Total comments: 2
Patch Set 26 : Fixing nit. #Patch Set 27 : Updating unit test. #Patch Set 28 : Rebase. #Patch Set 29 : Using WebContents URL. #Patch Set 30 : Fixing comment. #
Total comments: 18
Patch Set 31 : Addressing comments. #Patch Set 32 : Rebase + const. #Patch Set 33 : Fixing border. #Patch Set 34 : Rebuilding on 14039004. #
Total comments: 27
Patch Set 35 : Merging and addressing comments. #Patch Set 36 : Removing instant_source.{cc,h} #Patch Set 37 : Putting back CSS fixes. #Patch Set 38 : Rebase. #Patch Set 39 : virtual #Messages
Total messages: 91 (0 generated)
Please review. Still working on tests.
https://codereview.chromium.org/13375003/diff/1/chrome/browser/browser_resour... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/13375003/diff/1/chrome/browser/browser_resour... chrome/browser/browser_resources.grd:164: <include name="IDR_OMNIBOX_RESULT_JS" file="resources\omnibox_result.js" type="BINDATA" /> omnibox_result.js is missing from the CL. https://codereview.chromium.org/13375003/diff/1/chrome/browser/resources/omni... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/1/chrome/browser/resources/omni... chrome/browser/resources/omnibox_result_loader.js:83: updateResult(window.parent.frames[id].document, suggestion, Where's the "frames" array defined?
https://codereview.chromium.org/13375003/diff/1/chrome/browser/browser_resour... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/13375003/diff/1/chrome/browser/browser_resour... chrome/browser/browser_resources.grd:164: <include name="IDR_OMNIBOX_RESULT_JS" file="resources\omnibox_result.js" type="BINDATA" /> On 2013/04/01 21:03:27, sreeram wrote: > omnibox_result.js is missing from the CL. Oops, sorry! https://codereview.chromium.org/13375003/diff/1/chrome/browser/resources/omni... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/1/chrome/browser/resources/omni... chrome/browser/resources/omnibox_result_loader.js:83: updateResult(window.parent.frames[id].document, suggestion, On 2013/04/01 21:03:27, sreeram wrote: > Where's the "frames" array defined? This is referencing window.frames https://developer.mozilla.org/en-US/docs/DOM/window.frames
https://codereview.chromium.org/13375003/diff/1/chrome/browser/resources/omni... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/1/chrome/browser/resources/omni... chrome/browser/resources/omnibox_result_loader.js:83: updateResult(window.parent.frames[id].document, suggestion, On 2013/04/01 21:29:18, Jered wrote: > On 2013/04/01 21:03:27, sreeram wrote: > > Where's the "frames" array defined? > > This is referencing window.frames > https://developer.mozilla.org/en-US/docs/DOM/window.frames Correct me if I am wrong: window.parent refers to the embedder page, right? How does this work across origins? (This script is executing in a chrome-search://suggestion/ origin, whereas the embedder is either https://www.google.com/ or chrome-search://local-omnibox-popup/.)
On 2013/04/01 21:50:40, sreeram wrote: > https://codereview.chromium.org/13375003/diff/1/chrome/browser/resources/omni... > File chrome/browser/resources/omnibox_result_loader.js (right): > > https://codereview.chromium.org/13375003/diff/1/chrome/browser/resources/omni... > chrome/browser/resources/omnibox_result_loader.js:83: > updateResult(window.parent.frames[id].document, suggestion, > On 2013/04/01 21:29:18, Jered wrote: > > On 2013/04/01 21:03:27, sreeram wrote: > > > Where's the "frames" array defined? > > > > This is referencing window.frames > > https://developer.mozilla.org/en-US/docs/DOM/window.frames > > Correct me if I am wrong: window.parent refers to the embedder page, right? Correct. > How > does this work across origins? (This script is executing in a > chrome-search://suggestion/ origin, whereas the embedder is either > https://www.google.com/ or chrome-search://local-omnibox-popup/.) The nested browsing context is allowed to access certain properties on the parent window object (frames, length, top, etc.) This is necessary for postMessage() to work.
The rebase in PS3 leaves out the field trial; we can still merge that in M27, which will suck without it, but I think with this change we do not need it in trunk.
Haven't made it all the way through yet, but here are some initial comments. Thanks, Samarth https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:143: /** Not sure what style Chrome JS prefers, but I would find it cleaner to split off the helpers classes into a separate file. Edit: I guess this would complicate things a bit because you need to "register" all new files with in source .cc/.h files. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:162: iframe.addEventListener('mouseover', hover(iframe.id), false); It's not clear at all that hover() returns a function. I think it's only used here so I'd just either inline it or rename it to something clearer. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:246: $('.' + this.containerClass).innerHTML = ''; This file uses a mix of raw DOM operations (see IframePool init) and sizzle style selectors. Let's pick one and stick to it. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:420: ' .suggestion:nth-of-type(' + (this.hoveredIndex_ + 1) + ')'); A lot of code like this could be cleaned up if the suggestions box kept an array of Suggestion objects and the suggestion was responsible for doing things like updating hover and selected states. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:436: activeSuggestionsBox.containerClass = 'active-container'; This is a little weird. The suggestions box is already the one creating the container. Why should this code even know about the active vs pending class name? Can't we just call some pendingBox.activate(); and activeBox.deactivate() methods to do this work? https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... File chrome/browser/resources/omnibox_result.html (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result.html:24: <span id="url"></span> In the other search provider case, are these iframes also showing queries? I'd try to make that more obvious. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... File chrome/browser/resources/omnibox_result.js (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result.js:19: window.addEventListener('click', handleClick, false); Strictly speaking you shouldn't need to wait for DOMContentLoaded to add listeners to window, but this is fine. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result_loader.js:26: * @param {Object} userStyle User-specified overrides for suggestion styles. "User" is confusing here. How about pageStyle or something to make it clear it's being set by the instant page. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result_loader.js:34: font: apiHandle.font, Any reason we don't want to allow the page to override these as well? I can imagine wanting to run experiments that change the suggest font size. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result_loader.js:53: urlNode.textContent = suggestion.destination_url; As mentioned in a different comment, this needs to support queries as well for other search providers. https://codereview.chromium.org/13375003/diff/24/chrome/browser/search/sugges... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/search/sugges... chrome/browser/search/suggestion_source.cc:30: std::string StripQueryString(const std::string& path) { Is there an easy way of doing with some existing utility class (maybe GURL)? I'd prefer not to implement this ourselves. https://codereview.chromium.org/13375003/diff/24/chrome/browser/search/sugges... File chrome/browser/search/suggestion_source.h (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/search/sugges... chrome/browser/search/suggestion_source.h:19: // Overridden from content::URLDataSource: These can be private. https://codereview.chromium.org/13375003/diff/24/chrome/renderer/chrome_conte... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/13375003/diff/24/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:992: // Note that this replaces any existing &origin param. This also replaces any other query params, but I suppose that is OK. You should probably note that as well.
PTAL for JS samarth, I have cleaned up a bit. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:143: /** On 2013/04/02 23:26:58, samarth wrote: > Not sure what style Chrome JS prefers, but I would find it cleaner to split off > the helpers classes into a separate file. > > Edit: I guess this would complicate things a bit because you need to "register" > all new files with in source .cc/.h files. Yeah, ideally we'd have a compiler of some sort... I'd rather keep this in one file given we don't have that. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:162: iframe.addEventListener('mouseover', hover(iframe.id), false); On 2013/04/02 23:26:58, samarth wrote: > It's not clear at all that hover() returns a function. I think it's only used > here so I'd just either inline it or rename it to something clearer. Done. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:246: $('.' + this.containerClass).innerHTML = ''; On 2013/04/02 23:26:58, samarth wrote: > This file uses a mix of raw DOM operations (see IframePool init) and sizzle > style selectors. Let's pick one and stick to it. Leaving as is based on our offline conversation, since this is just selecting an existing element. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:420: ' .suggestion:nth-of-type(' + (this.hoveredIndex_ + 1) + ')'); On 2013/04/02 23:26:58, samarth wrote: > A lot of code like this could be cleaned up if the suggestions box kept an array > of Suggestion objects and the suggestion was responsible for doing things like > updating hover and selected states. Done. I still found it cleaner to have separate redrawSelection_ and redrawHover_ methods, though. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/loc... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:436: activeSuggestionsBox.containerClass = 'active-container'; On 2013/04/02 23:26:58, samarth wrote: > This is a little weird. The suggestions box is already the one creating the > container. Why should this code even know about the active vs pending class > name? Can't we just call some pendingBox.activate(); and activeBox.deactivate() > methods to do this work? Done, sort of. I'm not sure this is less confusing though? Initially, there is no active suggestions box. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... File chrome/browser/resources/omnibox_result.html (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result.html:24: <span id="url"></span> On 2013/04/02 23:26:58, samarth wrote: > In the other search provider case, are these iframes also showing queries? I'd > try to make that more obvious. Per offline discussion, they're currently showing search URLs. Would you prefer I clean this up now or in a follow on change? https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... File chrome/browser/resources/omnibox_result.js (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result.js:19: window.addEventListener('click', handleClick, false); On 2013/04/02 23:26:58, samarth wrote: > Strictly speaking you shouldn't need to wait for DOMContentLoaded to add > listeners to window, but this is fine. Ack. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result_loader.js:26: * @param {Object} userStyle User-specified overrides for suggestion styles. On 2013/04/02 23:26:58, samarth wrote: > "User" is confusing here. How about pageStyle or something to make it clear > it's being set by the instant page. Done. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result_loader.js:26: * @param {Object} userStyle User-specified overrides for suggestion styles. On 2013/04/02 23:26:58, samarth wrote: > "User" is confusing here. How about pageStyle or something to make it clear > it's being set by the instant page. Done. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result_loader.js:34: font: apiHandle.font, On 2013/04/02 23:26:58, samarth wrote: > Any reason we don't want to allow the page to override these as well? I can > imagine wanting to run experiments that change the suggest font size. I was thinking we'd always want suggestions to have the same font styling as the omnibox, since we went to some pains to get into that state. Also allowing font and size brings along another can of CSS escaping issues I'd rather deal with only if we need to. https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result_loader.js:53: urlNode.textContent = suggestion.destination_url; On 2013/04/02 23:26:58, samarth wrote: > As mentioned in a different comment, this needs to support queries as well for > other search providers. Done.
https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... File chrome/browser/resources/omnibox_result.html (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/resources/omn... chrome/browser/resources/omnibox_result.html:24: <span id="url"></span> On 2013/04/03 18:49:33, Jered wrote: > On 2013/04/02 23:26:58, samarth wrote: > > In the other search provider case, are these iframes also showing queries? I'd > > try to make that more obvious. > > Per offline discussion, they're currently showing search URLs. Would you prefer > I clean this up now or in a follow on change? Oh never mind I ended up doing this.
PTAL https://codereview.chromium.org/13375003/diff/24/chrome/browser/search/sugges... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/search/sugges... chrome/browser/search/suggestion_source.cc:30: std::string StripQueryString(const std::string& path) { On 2013/04/02 23:26:58, samarth wrote: > Is there an easy way of doing with some existing utility class (maybe GURL)? I'd > prefer not to implement this ourselves. Seems wiser. Done. https://codereview.chromium.org/13375003/diff/24/chrome/browser/search/sugges... File chrome/browser/search/suggestion_source.h (right): https://codereview.chromium.org/13375003/diff/24/chrome/browser/search/sugges... chrome/browser/search/suggestion_source.h:19: // Overridden from content::URLDataSource: On 2013/04/02 23:26:58, samarth wrote: > These can be private. Done. https://codereview.chromium.org/13375003/diff/24/chrome/renderer/chrome_conte... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/13375003/diff/24/chrome/renderer/chrome_conte... chrome/renderer/chrome_content_renderer_client.cc:992: // Note that this replaces any existing &origin param. On 2013/04/02 23:26:58, samarth wrote: > This also replaces any other query params, but I suppose that is OK. You should > probably note that as well. Done.
Please review (broadening). palmer -> epic security review brettw -> content_renderer_client, chrome_browser.gypi, renderer/resources dbeam -> browser/resources
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css:12: .suggestionsBox { suggesions-box https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:119: function $(selector) { please change this to $$(), IMO https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:172: iframe.id = 'suggestion-text-' + i; ^ what's the point of this? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:175: iframe.addEventListener('mouseover', (function(id) { iframe.addEventListener('mouseover', function(e) { if (active) active.hover(e.currentTarget.id); }); https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:175: iframe.addEventListener('mouseover', (function(id) { why do you need to do hover/mouseout in JS? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:176: active && active.hover(id); s/active &&/if (active)/ https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:182: this.iframes_[i] = iframe; this.iframes_.push(iframe); https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:190: IframePool.prototype.get = function() { this seems like a bad name (seems like it's a const getter or something), can you change to .reserve() or .acquire() or something? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:200: iframe.style.top = OFF_SCREEN; why does this need to be offscreen rather than [hidden]? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:212: * @type {Object} should this be !Object? (and/or should you be doing assert(data))? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:223: this.div_; unless you guys are compiling this, this is pretty useless... https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:260: Suggestion.prototype.setIframe = function(iframe) { any reason these aren't public members or ES5 getter/setters? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:280: this.iframe_.style.top = (this.div_.offsetTop + 4) + 'px'; nit: unnecessary () https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:281: this.iframe_.style[isRtl ? 'right' : 'left'] = startMargin + 'px'; it's arguable that you should just style both things, i.e. this.iframe_.style.left = startMargin + 'px'; // will work in LTR. this.iframe_.style.right = startMargin + 'px'; // overrides in RTL. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:324: * @param {Array.<Object>} suggestionData Suggestion data to display. can this be null or have null objects? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:365: this.suggestions_[i] = new Suggestion(suggestionData[i]); nit: curlies, .push() https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:380: this.container_ = $('.pending-container'); assert(this.container_); maybe? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:388: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) can suggestions be null? is so this could skip some suggestions from being destroy()ed https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:389: suggestion.destroy(); nit: curlies https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:400: box.className = 'suggestionsBox'; suggestions-box https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:424: * Returns whether the response is for the suggestions in this box. ^ doesn't seem necessary https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:426: * @return {boolean} True if the request is for the suggestions in this box and Whether the request is for the suggestions in this box https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:443: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) why are you doing this everywhere vs. suggestions.forEach(function(suggestion, i) { ? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:444: suggestion.reposition(isRtl, startMargin, totalMargin); same nit re: curlies https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:454: this.selectedIndex_ = -1; nit: \n https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:468: this.redrawHover_(); nit: seems this could share code with selectPrevious https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:490: * @param {Window} iframeWindow The window of the iframe that was clicked. can Window be null? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:491: * @return {number?} The restricted id clicked or null if none. ?number I think is more common https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:494: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; ++i) curlies https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:523: this.redrawHover_(); this.clearHover(); https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:663: $('head').appendChild(style); $qs() would be useful here as if you're looking at other webui code this would mean #head, not <head> https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:674: if (active) { nit: if (!active) return; switch (e.keyIdentifier) { case 'Up': ... case 'Down': ... https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:699: } else if ('click' in message.data) { } else if ('click' in message.data && active) { https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:702: if (restrictedId != null) { if (restrictedId) (if possible) https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:714: function init() { ^ can you take these out of the global namespace? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:734: active && active.clearHover(); if (active) https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result.html (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:8: cursor: default; ^ er, this should be the default... no? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:13: } \n https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:18: } \n https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:19: .hide { display: none; } why are you not using [hidden] instead? if this stays .hide { display: none; } https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:1: /** does this need a copyright? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:7: * Must be in double quotes for proper escaping. what must be in double quotes? please clarify https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:15: top.postMessage(clickMessage, EMBEDDER_ORIGIN); top.postMessage({click: event.button}, EMBEDDER_ORIGIN); https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:18: document.addEventListener('DOMContentLoaded', function() { why do you need to wait until DOMContentLoaded? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:20: }); why is this all in the global namespace? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:12: var EMBEDDER_ORIGIN = "%s"; can you share with with the other file? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:21: return /^#[0-9A-Fa-f]{3}|#[0-9A-Fa-f]{6}$/.test(color); '\m/d(^o^)b\m/#123456' passes, '#123_OMGWTFBBQ' passes return /^#([0-9a-f]{3}|[0-9a-f]{6})$/i.test(color); tests? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:28: * @return {Object} Checked styles or defaults. ^ can these be null? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:52: * @param {Object} style Checked (not user-set) result style. ^ can any of these be null? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:81: return; \n https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:93: {'loaded': message.data.requestId}, nit: this could probably all fit in one line https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:96: } why are these in the global namespace?
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:46: var QUERY_COLOR = '#000'; NIT: const instead of var? The internet tells me Chrome supports that. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:276: * @param {number} totalMargin Total non-content space on suggestion line. Enforce that |startMargin| and |totalMargin| are numbers, since they get interpolated into CSS? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:21: return /^#[0-9A-Fa-f]{3}|#[0-9A-Fa-f]{6}$/.test(color); This is too permissive. Express color as an integer, and format it upon interpolation? Security pattern: Strings are blobs; prefer specific and primitive types to generic, blobular thingies. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:55: resultDoc.body.style.font = style.fontSize + 'px "' + style.font + '"'; Can this function be absolutely sure that |font| and |fontSize| are safe for interpolation into CSS? (For example, can this function be sure that style is the return value of |getStyle|?) Currently, the one caller (|handleMessage|) does send the return value of |getStyle|. It'd be more robust against future changes to let callers pass whatever they want, and to have |updateResult| run the style through |getStyle|. Security pattern: Enforce policy as closely as possible to where it's needed. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/search/sug... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/search/sug... chrome/browser/search/suggestion_source.cc:106: if (request->url().SchemeIs(chrome::kChromeSearchScheme)) { NIT: You could rewrite this, with less indentation, as: const std::string& path = request->url().path(); return request->url().SchemeIs(chrome::kChromeSearchScheme) && (path == kLoaderHtmlPath || path == kLoaderJSPath || path == kResultHtmlPath || path == kResultJSPath); FWIW. https://codereview.chromium.org/13375003/diff/41001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/13375003/diff/41001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:996: // Note that this replaces any existing parameters, including any existing "...any existing URL query string parameters..." (took me an additional microsecond to figure out what kind of parameter you meant). And why should the consumer of this URL believe this new origin parameter instead of any other one? Might this break something somewhere that needed a parameter named origin? https://codereview.chromium.org/13375003/diff/41001/chrome/renderer/resources... File chrome/renderer/resources/extensions/searchbox_api.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/renderer/resources... chrome/renderer/resources/extensions/searchbox_api.js:239: // This method is restricted to chrome-search://suggestion pages. What enforces that restriction?
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:46: var QUERY_COLOR = '#000'; On 2013/04/05 01:30:32, Chris P. wrote: > NIT: const instead of var? The internet tells me Chrome supports that. no, don't do that - const is deprected, banned, non-standard, and slow. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:21: return /^#[0-9A-Fa-f]{3}|#[0-9A-Fa-f]{6}$/.test(color); On 2013/04/05 01:30:32, Chris P. wrote: > This is too permissive. ^ what can you do with this? > > Express color as an integer, and format it upon interpolation? > > Security pattern: Strings are blobs; prefer specific and primitive types to > generic, blobular thingies. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:55: resultDoc.body.style.font = style.fontSize + 'px "' + style.font + '"'; On 2013/04/05 01:30:32, Chris P. wrote: > Can this function be absolutely sure that |font| and |fontSize| are safe for > interpolation into CSS? (For example, can this function be sure that style is > the return value of |getStyle|?) > > Currently, the one caller (|handleMessage|) does send the return value of > |getStyle|. It'd be more robust against future changes to let callers pass > whatever they want, and to have |updateResult| run the style through |getStyle|. > > Security pattern: Enforce policy as closely as possible to where it's needed. resultDoc.body.style.fontSize = style.fontSize + 'px'; resultDoc.body.style.fontFamily = style.font; may help
Thanks! Addressed most comments, but had some questions below. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css:12: .suggestionsBox { On 2013/04/05 01:07:37, Dan Beam wrote: > suggesions-box Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:46: var QUERY_COLOR = '#000'; On 2013/04/05 01:38:45, Dan Beam wrote: > On 2013/04/05 01:30:32, Chris P. wrote: > > NIT: const instead of var? The internet tells me Chrome supports that. > > no, don't do that - const is deprected, banned, non-standard, and slow. Not done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:46: var QUERY_COLOR = '#000'; On 2013/04/05 01:30:32, Chris P. wrote: > NIT: const instead of var? The internet tells me Chrome supports that. Going with dbeam's advice. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:119: function $(selector) { On 2013/04/05 01:07:37, Dan Beam wrote: > please change this to $$(), IMO Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:172: iframe.id = 'suggestion-text-' + i; On 2013/04/05 01:07:37, Dan Beam wrote: > ^ what's the point of this? ids are passed via postMessage() to the loader iframe to tell it which frames from parent.frames to load. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:175: iframe.addEventListener('mouseover', (function(id) { On 2013/04/05 01:07:37, Dan Beam wrote: > iframe.addEventListener('mouseover', function(e) { > if (active) > active.hover(e.currentTarget.id); > }); Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:175: iframe.addEventListener('mouseover', (function(id) { On 2013/04/05 01:07:37, Dan Beam wrote: > why do you need to do hover/mouseout in JS? The element hovered is an absolutely positioned* iframe but we want to draw the hover with a div positioned behind it**. The iframe has no fixed dom relationship to the hover div, so is not really amenable to CSS styling. * The iframes must be absolutely positioned because removing an iframe from the dom and adding it back reloads it, which makes the load spinner flash with every keystroke. ** An alternative would be to make iframes large enough to encompass the entire hover region and add a body:hover in the iframes, but then they'd also have to know about selection so they don't show a hover when selected with the keyboard (which is not quite the same as being focused, in the usual sense), which would involve more postMessages, so we'd also need Javascript for that approach. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:176: active && active.hover(id); On 2013/04/05 01:07:37, Dan Beam wrote: > s/active &&/if (active)/ Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:182: this.iframes_[i] = iframe; On 2013/04/05 01:07:37, Dan Beam wrote: > this.iframes_.push(iframe); Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:190: IframePool.prototype.get = function() { On 2013/04/05 01:07:37, Dan Beam wrote: > this seems like a bad name (seems like it's a const getter or something), can > you change to .reserve() or .acquire() or something? Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:200: iframe.style.top = OFF_SCREEN; On 2013/04/05 01:07:37, Dan Beam wrote: > why does this need to be offscreen rather than [hidden]? No particular reason. I think visibility: hidden would probably work ok. Do you have a strong preference, given the element must be repositioned to show up in the proper place, anyway? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:212: * @type {Object} On 2013/04/05 01:07:37, Dan Beam wrote: > should this be !Object? (and/or should you be doing assert(data))? Done. Where is assert() defined? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:223: this.div_; On 2013/04/05 01:07:37, Dan Beam wrote: > unless you guys are compiling this, this is pretty useless... Force of habit. Removed. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:260: Suggestion.prototype.setIframe = function(iframe) { On 2013/04/05 01:07:37, Dan Beam wrote: > any reason these aren't public members or ES5 getter/setters? Just habit. What would be most consistent with webui style? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:276: * @param {number} totalMargin Total non-content space on suggestion line. On 2013/04/05 01:30:32, Chris P. wrote: > Enforce that |startMargin| and |totalMargin| are numbers, since they get > interpolated into CSS? It looks bizarre to me, but I've done this. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:280: this.iframe_.style.top = (this.div_.offsetTop + 4) + 'px'; On 2013/04/05 01:07:37, Dan Beam wrote: > nit: unnecessary () Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:281: this.iframe_.style[isRtl ? 'right' : 'left'] = startMargin + 'px'; On 2013/04/05 01:07:37, Dan Beam wrote: > it's arguable that you should just style both things, i.e. > > this.iframe_.style.left = startMargin + 'px'; // will work in LTR. > this.iframe_.style.right = startMargin + 'px'; // overrides in RTL. Huh? How would that work and take into account apiHandle.isRtl? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:324: * @param {Array.<Object>} suggestionData Suggestion data to display. On 2013/04/05 01:07:37, Dan Beam wrote: > can this be null or have null objects? Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:365: this.suggestions_[i] = new Suggestion(suggestionData[i]); On 2013/04/05 01:07:37, Dan Beam wrote: > nit: curlies, .push() Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:380: this.container_ = $('.pending-container'); On 2013/04/05 01:07:37, Dan Beam wrote: > assert(this.container_); maybe? Please see above. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:388: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) On 2013/04/05 01:07:37, Dan Beam wrote: > can suggestions be null? is so this could skip some suggestions from being > destroy()ed No. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:389: suggestion.destroy(); On 2013/04/05 01:07:37, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:400: box.className = 'suggestionsBox'; On 2013/04/05 01:07:37, Dan Beam wrote: > suggestions-box Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:424: * Returns whether the response is for the suggestions in this box. On 2013/04/05 01:07:37, Dan Beam wrote: > ^ doesn't seem necessary Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:426: * @return {boolean} True if the request is for the suggestions in this box and On 2013/04/05 01:07:37, Dan Beam wrote: > Whether the request is for the suggestions in this box Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:443: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) On 2013/04/05 01:07:37, Dan Beam wrote: > why are you doing this everywhere vs. > > suggestions.forEach(function(suggestion, i) { > > ? Is forEach more appropriate for webui Javascript? I don't see this a lot elsewhere in google Javascript. I'm in the habit of writing loops this way for collections that don't contain nulls due to [go/js-style "Iterating over Node Lists"]. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:444: suggestion.reposition(isRtl, startMargin, totalMargin); On 2013/04/05 01:07:37, Dan Beam wrote: > same nit re: curlies Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:454: this.selectedIndex_ = -1; On 2013/04/05 01:07:37, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:468: this.redrawHover_(); On 2013/04/05 01:07:37, Dan Beam wrote: > nit: seems this could share code with selectPrevious Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:490: * @param {Window} iframeWindow The window of the iframe that was clicked. On 2013/04/05 01:07:37, Dan Beam wrote: > can Window be null? Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:491: * @return {number?} The restricted id clicked or null if none. On 2013/04/05 01:07:37, Dan Beam wrote: > ?number I think is more common Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:494: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; ++i) On 2013/04/05 01:07:37, Dan Beam wrote: > curlies Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:523: this.redrawHover_(); On 2013/04/05 01:07:37, Dan Beam wrote: > this.clearHover(); Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:663: $('head').appendChild(style); On 2013/04/05 01:07:37, Dan Beam wrote: > $qs() would be useful here as if you're looking at other webui code this would > mean #head, not <head> Is $$ ok? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:674: if (active) { On 2013/04/05 01:07:37, Dan Beam wrote: > nit: > > if (!active) > return; > > switch (e.keyIdentifier) { > case 'Up': > ... > case 'Down': > ... Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:699: } else if ('click' in message.data) { On 2013/04/05 01:07:37, Dan Beam wrote: > } else if ('click' in message.data && active) { My intent was to avoid ambiguity for future messages, so that a message with .click is always interpreted as this kind of message regardless of whether we're able to handle it. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:702: if (restrictedId != null) { On 2013/04/05 01:07:37, Dan Beam wrote: > if (restrictedId) (if possible) I think that 0 is a valid restrictedId. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:714: function init() { On 2013/04/05 01:07:37, Dan Beam wrote: > ^ can you take these out of the global namespace? By these, do you mean everything in this file? What namespace should I use? I'm not familiar with webui conventions. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:734: active && active.clearHover(); On 2013/04/05 01:07:37, Dan Beam wrote: > if (active) Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result.html (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:8: cursor: default; On 2013/04/05 01:07:37, Dan Beam wrote: > ^ er, this should be the default... no? This is here to avoid showing an I beam cursor over text inside the frame. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:13: } On 2013/04/05 01:07:37, Dan Beam wrote: > \n Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:18: } On 2013/04/05 01:07:37, Dan Beam wrote: > \n Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:19: .hide { display: none; } On 2013/04/05 01:07:37, Dan Beam wrote: > why are you not using [hidden] instead? if this stays > > .hide { > display: none; > } Sorry, what do you mean by [hidden]? visibility:hidden or an attribute selector? I've reformatted this though. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:1: /** On 2013/04/05 01:07:37, Dan Beam wrote: > does this need a copyright? Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:7: * Must be in double quotes for proper escaping. On 2013/04/05 01:07:37, Dan Beam wrote: > what must be in double quotes? please clarify Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:15: top.postMessage(clickMessage, EMBEDDER_ORIGIN); On 2013/04/05 01:07:37, Dan Beam wrote: > top.postMessage({click: event.button}, EMBEDDER_ORIGIN); Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:18: document.addEventListener('DOMContentLoaded', function() { On 2013/04/05 01:07:37, Dan Beam wrote: > why do you need to wait until DOMContentLoaded? I don't. Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:20: }); On 2013/04/05 01:07:37, Dan Beam wrote: > why is this all in the global namespace? Where would be better? (This script will be the only script alive in an iframe.) https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:12: var EMBEDDER_ORIGIN = "%s"; On 2013/04/05 01:07:37, Dan Beam wrote: > can you share with with the other file? Because I've got CSP disallowing inline script, I think I need separate js files for the loader and result iframes (because only the result iframes should bind a click handler e.g.) https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:21: return /^#[0-9A-Fa-f]{3}|#[0-9A-Fa-f]{6}$/.test(color); On 2013/04/05 01:07:37, Dan Beam wrote: > '\m/d(^o^)b\m/#123456' passes, '#123_OMGWTFBBQ' passes > > return /^#([0-9a-f]{3}|[0-9a-f]{6})$/i.test(color); > > tests? Sorry, I got this wrong. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:21: return /^#[0-9A-Fa-f]{3}|#[0-9A-Fa-f]{6}$/.test(color); On 2013/04/05 01:30:32, Chris P. wrote: > This is too permissive. > > Express color as an integer, and format it upon interpolation? > > Security pattern: Strings are blobs; prefer specific and primitive types to > generic, blobular thingies. That is a good pattern, but it's rather awkward in Javascript. What do you guys think of this? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:28: * @return {Object} Checked styles or defaults. On 2013/04/05 01:07:37, Dan Beam wrote: > ^ can these be null? Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:28: * @return {Object} Checked styles or defaults. On 2013/04/05 01:07:37, Dan Beam wrote: > ^ can these be null? Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:52: * @param {Object} style Checked (not user-set) result style. On 2013/04/05 01:07:37, Dan Beam wrote: > ^ can any of these be null? Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:55: resultDoc.body.style.font = style.fontSize + 'px "' + style.font + '"'; On 2013/04/05 01:38:45, Dan Beam wrote: > On 2013/04/05 01:30:32, Chris P. wrote: > > Can this function be absolutely sure that |font| and |fontSize| are safe for > > interpolation into CSS? (For example, can this function be sure that style is > > the return value of |getStyle|?) > > > > Currently, the one caller (|handleMessage|) does send the return value of > > |getStyle|. It'd be more robust against future changes to let callers pass > > whatever they want, and to have |updateResult| run the style through > |getStyle|. > > > > Security pattern: Enforce policy as closely as possible to where it's needed. > > resultDoc.body.style.fontSize = style.fontSize + 'px'; > resultDoc.body.style.fontFamily = style.font; > > may help Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:55: resultDoc.body.style.font = style.fontSize + 'px "' + style.font + '"'; On 2013/04/05 01:30:32, Chris P. wrote: > Can this function be absolutely sure that |font| and |fontSize| are safe for > interpolation into CSS? (For example, can this function be sure that style is > the return value of |getStyle|?) > > Currently, the one caller (|handleMessage|) does send the return value of > |getStyle|. It'd be more robust against future changes to let callers pass > whatever they want, and to have |updateResult| run the style through |getStyle|. > > Security pattern: Enforce policy as closely as possible to where it's needed. Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:81: return; On 2013/04/05 01:07:37, Dan Beam wrote: > \n Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:93: {'loaded': message.data.requestId}, On 2013/04/05 01:07:37, Dan Beam wrote: > nit: this could probably all fit in one line Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:96: } On 2013/04/05 01:07:37, Dan Beam wrote: > why are these in the global namespace? Let's discuss in other comment, and I'll fix it in the best way everywhere. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/search/sug... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/search/sug... chrome/browser/search/suggestion_source.cc:106: if (request->url().SchemeIs(chrome::kChromeSearchScheme)) { On 2013/04/05 01:30:32, Chris P. wrote: > NIT: You could rewrite this, with less indentation, as: > > const std::string& path = request->url().path(); > return request->url().SchemeIs(chrome::kChromeSearchScheme) && > (path == kLoaderHtmlPath || > path == kLoaderJSPath || > path == kResultHtmlPath || > path == kResultJSPath); > > FWIW. Done. Actually, strcmp is pretty cheap, so I'll make this DCHECK not a DCHECK, too. https://codereview.chromium.org/13375003/diff/41001/chrome/renderer/chrome_co... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/13375003/diff/41001/chrome/renderer/chrome_co... chrome/renderer/chrome_content_renderer_client.cc:996: // Note that this replaces any existing parameters, including any existing On 2013/04/05 01:30:32, Chris P. wrote: > "...any existing URL query string parameters..." (took me an additional > microsecond to figure out what kind of parameter you meant). Done. > > And why should the consumer of this URL believe this new origin parameter > instead of any other one? Because this origin is interpolated and checked as the allowed origin for postMessage(). > > Might this break something somewhere that needed a parameter named origin? Yes, but there is no such place. https://codereview.chromium.org/13375003/diff/41001/chrome/renderer/resources... File chrome/renderer/resources/extensions/searchbox_api.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/renderer/resources... chrome/renderer/resources/extensions/searchbox_api.js:239: // This method is restricted to chrome-search://suggestion pages. On 2013/04/05 01:30:32, Chris P. wrote: > What enforces that restriction? I elaborated on the comment.
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:674: if (active) { On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > nit: > > > > if (!active) > > return; > > > > switch (e.keyIdentifier) { > > case 'Up': > > ... > > case 'Down': > > ... > > Done. Oh, bummer, this does not work. e has only a keyCode member. I've revised this to use keyCode with named constants.
https://codereview.chromium.org/13375003/diff/60003/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/60003/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:282: // Call parseInt for security paranoia since we're interpolating CSS. s/security paranoia/enforcing your documented API guarantee in the above comments/g https://codereview.chromium.org/13375003/diff/60003/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/60003/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:23: function convertColor(color) { Much better, thanks.
https://codereview.chromium.org/13375003/diff/60003/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/60003/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:282: // Call parseInt for security paranoia since we're interpolating CSS. On 2013/04/05 18:43:40, Chris P. wrote: > s/security paranoia/enforcing your documented API guarantee in the above > comments/g Sorry, didn't mean any offense. Reworded to more clearly state why we're doing this. https://codereview.chromium.org/13375003/diff/60003/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/60003/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:23: function convertColor(color) { On 2013/04/05 18:43:40, Chris P. wrote: > Much better, thanks. Ack np.
"git apply ~/your-patch" fails, sadly, so I am having a hard time trying it out. After a fresh gclient sync: ~/chromium/src $ git status # On branch master nothing to commit, working directory clean ~/chromium/src $ git apply ~/issue13375003_76001.diff.txt error: patch failed: chrome/renderer/chrome_content_renderer_client.cc:11 error: chrome/renderer/chrome_content_renderer_client.cc: patch does not apply To be honest I am not sure why the patch doesn't apply; it seems fine to me at that point in the patch file.
On 2013/04/05 19:22:04, Chris P. wrote: > "git apply ~/your-patch" fails, sadly, so I am having a hard time trying it out. > After a fresh gclient sync: > > ~/chromium/src $ git status > # On branch master > nothing to commit, working directory clean > ~/chromium/src $ git apply ~/issue13375003_76001.diff.txt > error: patch failed: chrome/renderer/chrome_content_renderer_client.cc:11 > error: chrome/renderer/chrome_content_renderer_client.cc: patch does not apply > > To be honest I am not sure why the patch doesn't apply; it seems fine to me at > that point in the patch file. Just rebased; does this help?
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:119: function $(selector) { On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > please change this to $$(), IMO > > Done. oh, I'm sorry, I meant $qs(), $$() usually means more than one will be returned. sorry, I thought I updated this.
> Just rebased; does this help? Yes, thanks. Building now, and then I'll try it out.
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:119: function $(selector) { On 2013/04/05 21:58:49, Dan Beam wrote: > On 2013/04/05 15:30:23, Jered wrote: > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > please change this to $$(), IMO > > > > Done. > > oh, I'm sorry, I meant $qs(), $$() usually means more than one will be returned. > sorry, I thought I updated this. Done.
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:200: iframe.style.top = OFF_SCREEN; On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > why does this need to be offscreen rather than [hidden]? > > No particular reason. I think visibility: hidden would probably work ok. Do you > have a strong preference, given the element must be repositioned to show up in > the proper place, anyway? if it's display:none; the page renders more quickly (minutely?) as it's not in the render tree. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:212: * @type {Object} On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > should this be !Object? (and/or should you be doing assert(data))? > > Done. Where is assert() defined? ui/webui/resources/js/util.js https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:260: Suggestion.prototype.setIframe = function(iframe) { On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > any reason these aren't public members or ES5 getter/setters? > > Just habit. What would be most consistent with webui style? well, normal webui style would be: Suggestion.prototype = { set iframe(iframe) { this.iframe_ = iframe; }, }; but you're using more Closurey style, so I guess it'd be Suggestion.prototype.__defineSetter__('iframe', function(iframe) { this.iframe_ = iframe; }); https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:281: this.iframe_.style[isRtl ? 'right' : 'left'] = startMargin + 'px'; On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > it's arguable that you should just style both things, i.e. > > > > this.iframe_.style.left = startMargin + 'px'; // will work in LTR. > > this.iframe_.style.right = startMargin + 'px'; // overrides in RTL. > > Huh? How would that work and take into account apiHandle.isRtl? this works because you're setting the [dir] https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:443: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > why are you doing this everywhere vs. > > > > suggestions.forEach(function(suggestion, i) { > > > > ? > > Is forEach more appropriate for webui Javascript? I don't see this a lot > elsewhere in google Javascript. I'm in the habit of writing loops this way for > collections that don't contain nulls due to [go/js-style "Iterating over Node > Lists"]. this is an array, not a node list. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:674: if (active) { On 2013/04/05 15:51:39, Jered wrote: > On 2013/04/05 15:30:23, Jered wrote: > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > nit: > > > > > > if (!active) > > > return; > > > > > > switch (e.keyIdentifier) { > > > case 'Up': > > > ... > > > case 'Down': > > > ... > > > > Done. > > Oh, bummer, this does not work. e has only a keyCode member. I've revised this > to use keyCode with named constants. strange, that works for me right now. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:714: function init() { On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > ^ can you take these out of the global namespace? > > By these, do you mean everything in this file? What namespace should I use? I'm > not familiar with webui conventions. if it's not necessary to access them from anywhere else, just an anonymous namespace, i.e. (function() { ... }()); or if they do need to be exported, cr.define('namespace_unix_hacker', function() { ... return {PublishedGlobal: PublishedGlobal}; });
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result.html (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:19: .hide { display: none; } On 2013/04/05 15:30:23, Jered wrote: > On 2013/04/05 01:07:37, Dan Beam wrote: > > why are you not using [hidden] instead? if this stays > > > > .hide { > > display: none; > > } > > Sorry, what do you mean by [hidden]? visibility:hidden or an attribute selector? > I've reformatted this though. someEl.hidden = true; // in JS
Couple quick questions for you Dan before I have another go at the Javascript... https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:212: * @type {Object} On 2013/04/05 22:17:26, Dan Beam wrote: > On 2013/04/05 15:30:23, Jered wrote: > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > should this be !Object? (and/or should you be doing assert(data))? > > > > Done. Where is assert() defined? > > ui/webui/resources/js/util.js Sorry, I meant how do I get at that code from here? I tried adding an assert() but the method is undefined. The Javascript in this CL is maybe a bit unusual because it runs in its own special origin. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:260: Suggestion.prototype.setIframe = function(iframe) { On 2013/04/05 22:17:26, Dan Beam wrote: > On 2013/04/05 15:30:23, Jered wrote: > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > any reason these aren't public members or ES5 getter/setters? > > > > Just habit. What would be most consistent with webui style? > > well, normal webui style would be: > > Suggestion.prototype = { > set iframe(iframe) { this.iframe_ = iframe; }, > }; > > but you're using more Closurey style, so I guess it'd be > > Suggestion.prototype.__defineSetter__('iframe', function(iframe) { > this.iframe_ = iframe; > }); What style do you prefer? I'm happy to write webui style rather than closure style as I think the latter is awfully verbose, but have just gotten used to it. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:281: this.iframe_.style[isRtl ? 'right' : 'left'] = startMargin + 'px'; On 2013/04/05 22:17:26, Dan Beam wrote: > On 2013/04/05 15:30:23, Jered wrote: > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > it's arguable that you should just style both things, i.e. > > > > > > this.iframe_.style.left = startMargin + 'px'; // will work in LTR. > > > this.iframe_.style.right = startMargin + 'px'; // overrides in RTL. > > > > Huh? How would that work and take into account apiHandle.isRtl? > > this works because you're setting the [dir] I'm setting the dir but not on the iframe or on a parent element. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:443: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) On 2013/04/05 22:17:26, Dan Beam wrote: > On 2013/04/05 15:30:23, Jered wrote: > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > why are you doing this everywhere vs. > > > > > > suggestions.forEach(function(suggestion, i) { > > > > > > ? > > > > Is forEach more appropriate for webui Javascript? I don't see this a lot > > elsewhere in google Javascript. I'm in the habit of writing loops this way for > > collections that don't contain nulls due to [go/js-style "Iterating over Node > > Lists"]. > > this is an array, not a node list. What style do you prefer? https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:674: if (active) { On 2013/04/05 22:17:26, Dan Beam wrote: > On 2013/04/05 15:51:39, Jered wrote: > > On 2013/04/05 15:30:23, Jered wrote: > > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > > nit: > > > > > > > > if (!active) > > > > return; > > > > > > > > switch (e.keyIdentifier) { > > > > case 'Up': > > > > ... > > > > case 'Down': > > > > ... > > > > > > Done. > > > > Oh, bummer, this does not work. e has only a keyCode member. I've revised this > > to use keyCode with named constants. > > strange, that works for me right now. This keypress is actually some kind of doctored up mouse event I think, it comes from Instant and not from wherever those normally come from. It's got just keyCode.
Mostly minor comments below. LGTM. (Also, this version is ridiculously better than the baseline iframes approach!) Thanks, Samarth https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css:56: /* Styling for native suggestions, hidden by the shadow DOM. */ We can kill these styles, no? https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:254: this.div_ = div; If we are using closure style, then add this member to the constructor. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:281: this.iframe_.style.top = this.div_.offsetTop + 4 + 'px'; Can you add named constants for the 4 and 26 below? I would not be surprised if they depend on the platform. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:488: var oldSelection = this.container_.querySelector('.selected'); Optional: you could have looked up the existing selected suggestion object with selectedIndex_ and called select(false) on it to be consistent. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:554: oldHover.classList.remove('hovered'); Again, it's weird that you have a .hover() method on the suggestion but then sometimes explicitly change the class here. But I don't care all that much if it's a lot of work to change this. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:585: if (active) { Yay, I can understand this function now :) https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:647: if (shouldSelectSuggestion(suggestions[0], apiHandle.verbatim)) Optional: you can make this a method on the Suggestion object, so if (suggestions[0].shouldSelect(apiHandle.verbatim)) { } https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/resources... File chrome/renderer/resources/extensions/searchbox_api.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/resources... chrome/renderer/resources/extensions/searchbox_api.js:128: // TODO(shishir): Fix the naming violations (chrome_search -> Add a TODO here to delete the shadow DOM stuff. https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox_extension.cc:135: v8::Handle<v8::Object> GenerateNativeSuggestion( Comments for what this does, please. https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox_extension.cc:161: } // namespace nit: keep a blank line before this https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox_extension.cc:1238: url.host() == chrome::kChromeSearchSuggestionHost)) I've seen both styles in Chrome but I like braces when the condition is multi-line.
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:281: this.iframe_.style[isRtl ? 'right' : 'left'] = startMargin + 'px'; On 2013/04/05 22:28:26, Jered wrote: > On 2013/04/05 22:17:26, Dan Beam wrote: > > On 2013/04/05 15:30:23, Jered wrote: > > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > > it's arguable that you should just style both things, i.e. > > > > > > > > this.iframe_.style.left = startMargin + 'px'; // will work in LTR. > > > > this.iframe_.style.right = startMargin + 'px'; // overrides in RTL. > > > > > > Huh? How would that work and take into account apiHandle.isRtl? > > > > this works because you're setting the [dir] > > I'm setting the dir but not on the iframe or on a parent element. ah, ok, stick with your original plan then https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:443: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) On 2013/04/05 22:28:26, Jered wrote: > On 2013/04/05 22:17:26, Dan Beam wrote: > > On 2013/04/05 15:30:23, Jered wrote: > > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > > why are you doing this everywhere vs. > > > > > > > > suggestions.forEach(function(suggestion, i) { > > > > > > > > ? > > > > > > Is forEach more appropriate for webui Javascript? I don't see this a lot > > > elsewhere in google Javascript. I'm in the habit of writing loops this way > for > > > collections that don't contain nulls due to [go/js-style "Iterating over > Node > > > Lists"]. > > > > this is an array, not a node list. > > What style do you prefer? forEach() if I require a closure to a reference (i.e. trying to us something in an event handler and want to make sure it stays the same), otherwise: for (var i = 0; i < this.suggestions_.length; ++i) { this.suggestions_[i].doSomething(); }
as to general questions on style -- it's up to you and/or your project. most of
webui happened to copy paste whoever wrote the first files (probably arv@), so
they all used .prototype = {__proto__:, methods, getters/setters};. i can't
really make that choice for you, just look at local style vs. global style and
weigh your options. if I had to guess, you'd probably save some effort not
rewriting your code as it is now, but sure -- we also think it looks slightly
prettier all in one prototype.
On 2013/04/05 22:36:09, Dan Beam wrote:
> as to general questions on style -- it's up to you and/or your project. most
of
> webui happened to copy paste whoever wrote the first files (probably arv@), so
> they all used .prototype = {__proto__:, methods, getters/setters};. i can't
> really make that choice for you, just look at local style vs. global style and
> weigh your options. if I had to guess, you'd probably save some effort not
> rewriting your code as it is now, but sure -- we also think it looks slightly
> prettier all in one prototype.
PTAL. I rewrote it that way. When in Rome etc. Note that I don't have the cr.ui
code here so have just done my best and am not using cr.define or any of that
(which I gather is idiomatic elsewhere).
https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:281: this.iframe_.style[isRtl ? 'right' : 'left'] = startMargin + 'px'; On 2013/04/05 22:32:47, Dan Beam wrote: > On 2013/04/05 22:28:26, Jered wrote: > > On 2013/04/05 22:17:26, Dan Beam wrote: > > > On 2013/04/05 15:30:23, Jered wrote: > > > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > > > it's arguable that you should just style both things, i.e. > > > > > > > > > > this.iframe_.style.left = startMargin + 'px'; // will work in LTR. > > > > > this.iframe_.style.right = startMargin + 'px'; // overrides in RTL. > > > > > > > > Huh? How would that work and take into account apiHandle.isRtl? > > > > > > this works because you're setting the [dir] > > > > I'm setting the dir but not on the iframe or on a parent element. > > ah, ok, stick with your original plan then ack https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:443: for (var i = 0, suggestion; suggestion = this.suggestions_[i]; i++) On 2013/04/05 22:32:47, Dan Beam wrote: > On 2013/04/05 22:28:26, Jered wrote: > > On 2013/04/05 22:17:26, Dan Beam wrote: > > > On 2013/04/05 15:30:23, Jered wrote: > > > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > > > why are you doing this everywhere vs. > > > > > > > > > > suggestions.forEach(function(suggestion, i) { > > > > > > > > > > ? > > > > > > > > Is forEach more appropriate for webui Javascript? I don't see this a lot > > > > elsewhere in google Javascript. I'm in the habit of writing loops this way > > for > > > > collections that don't contain nulls due to [go/js-style "Iterating over > > Node > > > > Lists"]. > > > > > > this is an array, not a node list. > > > > What style do you prefer? > > forEach() if I require a closure to a reference (i.e. trying to us something in > an event handler and want to make sure it stays the same), otherwise: > > for (var i = 0; i < this.suggestions_.length; ++i) { > this.suggestions_[i].doSomething(); > } Ok, I've opted for the latter style for loop in this file. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:714: function init() { On 2013/04/05 22:17:26, Dan Beam wrote: > On 2013/04/05 15:30:23, Jered wrote: > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > ^ can you take these out of the global namespace? > > > > By these, do you mean everything in this file? What namespace should I use? > I'm > > not familiar with webui conventions. > > if it's not necessary to access them from anywhere else, just an anonymous > namespace, i.e. > > (function() { > ... > }()); > > or if they do need to be exported, > > cr.define('namespace_unix_hacker', function() { > ... > return {PublishedGlobal: PublishedGlobal}; > }); Done. https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result.html (right): https://codereview.chromium.org/13375003/diff/41001/chrome/browser/resources/... chrome/browser/resources/omnibox_result.html:19: .hide { display: none; } On 2013/04/05 22:19:54, Dan Beam wrote: > On 2013/04/05 15:30:23, Jered wrote: > > On 2013/04/05 01:07:37, Dan Beam wrote: > > > why are you not using [hidden] instead? if this stays > > > > > > .hide { > > > display: none; > > > } > > > > Sorry, what do you mean by [hidden]? visibility:hidden or an attribute > selector? > > I've reformatted this though. > > someEl.hidden = true; // in JS Done. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css:56: /* Styling for native suggestions, hidden by the shadow DOM. */ On 2013/04/05 22:30:25, samarth wrote: > We can kill these styles, no? Done.
https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:285: parseInt(startMargin, 10) + 'px'; what happens when this is NaN? https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:395: } doesn't this leave you with a bunch of destroyed suggestions? why not: while (this.suggestions_.length > 0) { this.suggestions_.pop().destroy(); } https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:473: index = -1; index = Math.min(numSuggestions - 1, Math.max(-1, index)); https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:687: return; nit: \n https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:25: (color % 1) == 0 && color >= 0 && color <= 0xffffff) { if (!isFinite(color) || color < 0 || color > 0xffffff) return null; also, what is the % 1 for? https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:27: return '#000000'.substr(0, 7 - hexColor.length) + hexColor; this is pretty gross, but at least add a comment saying "Pads with initial zeros (e.g. 0x0000ff.toString(16) yields 'ff')." https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:43: titleColor: '#666666', shouldn't these be 0x666666, etc.? https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:60: * @param {!Object} pageStyle Page-specificed styles. shouldn't you be doing something along the lines of @param {?Object=} opt_pageStyle Optional page-specified styles. (? mean it could be null, = means it could be undefined) if you need a pageStyle || {}? https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:10: (function() { <include src="../path/to/shared/util.js> https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:109: var active; could you rename this to activeBox or something? active just sounds like a boolean member. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:132: * @return {HTMLElement} matching selector. s/matching selector/The first element to match |selector| or null./ https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:392: this.container_ = $qs('.pending-container'); assert(this.container_); https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:403: } or alternatively this.suggestions_.length = 0; https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:412: this.container_.innerHTML = ''; nit: \n https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:415: this.container_.appendChild(box); nit: \n https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:423: } nit: \n https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:426: 'load': iframesToLoad, we generally don't quote keys, but I understand why you might coming from a compiled JS background... https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:644: if (pending) { nit: if (pending) pending.destroy(); pending = null; https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... File chrome/browser/resources/omnibox_result.js (right): https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:22: window.addEventListener('click', handleClick, false); you could probably just inline if you'd like window.addEventListener('click', function(event) { top.postMessage({click: event.button}, EMBEDDER_ORIGIN); }); https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:71: var optionalNode = resultDoc.querySelector('#optional'); optionalNode.hidden = !suggestion.description;
Ping brettw? I'd like to be sure this content change is ok. Thanks for the detailed reviews so far. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:254: this.div_ = div; On 2013/04/05 22:30:25, samarth wrote: > If we are using closure style, then add this member to the constructor. Not using closure style. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:281: this.iframe_.style.top = this.div_.offsetTop + 4 + 'px'; On 2013/04/05 22:30:25, samarth wrote: > Can you add named constants for the 4 and 26 below? I would not be surprised if > they depend on the platform. Done. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:285: parseInt(startMargin, 10) + 'px'; On 2013/04/06 00:18:12, Dan Beam wrote: > what happens when this is NaN? Changed to set this style only if finite. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:395: } On 2013/04/06 00:18:12, Dan Beam wrote: > doesn't this leave you with a bunch of destroyed suggestions? why not: > > while (this.suggestions_.length > 0) { > this.suggestions_.pop().destroy(); > } Done. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:473: index = -1; On 2013/04/06 00:18:12, Dan Beam wrote: > index = Math.min(numSuggestions - 1, Math.max(-1, index)); Done. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:488: var oldSelection = this.container_.querySelector('.selected'); On 2013/04/05 22:30:25, samarth wrote: > Optional: you could have looked up the existing selected suggestion object with > selectedIndex_ and called select(false) on it to be consistent. I like this way because I can tell for sure there'll only be one thing selected, even if there's some bug with selectedIndex_. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:554: oldHover.classList.remove('hovered'); On 2013/04/05 22:30:25, samarth wrote: > Again, it's weird that you have a .hover() method on the suggestion but then > sometimes explicitly change the class here. But I don't care all that much if > it's a lot of work to change this. See above. It's mostly paranoia from past uis that had bugs with multiple selections. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:585: if (active) { On 2013/04/05 22:30:25, samarth wrote: > Yay, I can understand this function now :) Awesome! https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:647: if (shouldSelectSuggestion(suggestions[0], apiHandle.verbatim)) On 2013/04/05 22:30:25, samarth wrote: > Optional: you can make this a method on the Suggestion object, so > if (suggestions[0].shouldSelect(apiHandle.verbatim)) { > } Alas, this is suggestionData, not a Suggestion object. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:687: return; On 2013/04/06 00:18:12, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:25: (color % 1) == 0 && color >= 0 && color <= 0xffffff) { On 2013/04/06 00:18:12, Dan Beam wrote: > if (!isFinite(color) || color < 0 || color > 0xffffff) > return null; I'll keep the typeof check for extra sanity if you don't mind. > > also, what is the % 1 for? Colors cannot have fractional parts. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:27: return '#000000'.substr(0, 7 - hexColor.length) + hexColor; On 2013/04/06 00:18:12, Dan Beam wrote: > this is pretty gross, but at least add a comment saying "Pads with initial zeros > (e.g. 0x0000ff.toString(16) yields 'ff')." Done. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:43: titleColor: '#666666', On 2013/04/06 00:18:12, Dan Beam wrote: > shouldn't these be 0x666666, etc.? No, these are the safe fallback values which will be interpolated as written. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:60: * @param {!Object} pageStyle Page-specificed styles. On 2013/04/06 00:18:12, Dan Beam wrote: > shouldn't you be doing something along the lines of > > @param {?Object=} opt_pageStyle Optional page-specified styles. > > (? mean it could be null, = means it could be undefined) > > if you need a pageStyle || {}? I don't need that because the only caller of updateResult ensures pageStyle is not null. Removed. https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/resources... File chrome/renderer/resources/extensions/searchbox_api.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/resources... chrome/renderer/resources/extensions/searchbox_api.js:128: // TODO(shishir): Fix the naming violations (chrome_search -> On 2013/04/05 22:30:25, samarth wrote: > Add a TODO here to delete the shadow DOM stuff. Shishir has that TODO above. https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/searchbox... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox_extension.cc:135: v8::Handle<v8::Object> GenerateNativeSuggestion( On 2013/04/05 22:30:25, samarth wrote: > Comments for what this does, please. Done. https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox_extension.cc:161: } // namespace On 2013/04/05 22:30:25, samarth wrote: > nit: keep a blank line before this Done. https://codereview.chromium.org/13375003/diff/60009/chrome/renderer/searchbox... chrome/renderer/searchbox/searchbox_extension.cc:1238: url.host() == chrome::kChromeSearchSuggestionHost)) On 2013/04/05 22:30:25, samarth wrote: > I've seen both styles in Chrome but I like braces when the condition is > multi-line. Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:10: (function() { On 2013/04/06 00:18:13, Dan Beam wrote: > <include src="../path/to/shared/util.js> I get an error about an unrecognized < token. What's supposed to do this including and is it worth the effort to hook it up here? https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:109: var active; On 2013/04/06 00:18:13, Dan Beam wrote: > could you rename this to activeBox or something? active just sounds like a > boolean member. Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:132: * @return {HTMLElement} matching selector. On 2013/04/06 00:18:13, Dan Beam wrote: > s/matching selector/The first element to match |selector| or null./ Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:392: this.container_ = $qs('.pending-container'); On 2013/04/06 00:18:13, Dan Beam wrote: > assert(this.container_); See above. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:403: } On 2013/04/06 00:18:13, Dan Beam wrote: > or alternatively this.suggestions_.length = 0; Did the while loop thing. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:412: this.container_.innerHTML = ''; On 2013/04/06 00:18:13, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:415: this.container_.appendChild(box); On 2013/04/06 00:18:13, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:423: } On 2013/04/06 00:18:13, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:426: 'load': iframesToLoad, On 2013/04/06 00:18:13, Dan Beam wrote: > we generally don't quote keys, but I understand why you might coming from a > compiled JS background... Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:644: if (pending) { On 2013/04/06 00:18:13, Dan Beam wrote: > nit: > > if (pending) > pending.destroy(); > pending = null; Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... File chrome/browser/resources/omnibox_result.js (right): https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/omnibox_result.js:22: window.addEventListener('click', handleClick, false); On 2013/04/06 00:18:13, Dan Beam wrote: > you could probably just inline if you'd like > > window.addEventListener('click', function(event) { > top.postMessage({click: event.button}, EMBEDDER_ORIGIN); > }); Done. https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:71: var optionalNode = resultDoc.querySelector('#optional'); On 2013/04/06 00:18:13, Dan Beam wrote: > optionalNode.hidden = !suggestion.description; Done.
https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:25: (color % 1) == 0 && color >= 0 && color <= 0xffffff) { On 2013/04/06 02:45:16, Jered wrote: > On 2013/04/06 00:18:12, Dan Beam wrote: > > if (!isFinite(color) || color < 0 || color > 0xffffff) > > return null; > I'll keep the typeof check for extra sanity if you don't mind. > > > > also, what is the % 1 for? > Colors cannot have fractional parts. Math.floor() or Math.round() or assert(color % 1 == 0) or assert(Math.floor(color) == color); https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:43: titleColor: '#666666', On 2013/04/06 02:45:16, Jered wrote: > On 2013/04/06 00:18:12, Dan Beam wrote: > > shouldn't these be 0x666666, etc.? > > No, these are the safe fallback values which will be interpolated as written. ah, I see, sorry https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:10: (function() { On 2013/04/06 02:45:16, Jered wrote: > On 2013/04/06 00:18:13, Dan Beam wrote: > > <include src="../path/to/shared/util.js> > > I get an error about an unrecognized < token. What's supposed to do this > including and is it worth the effort to hook it up here? files should be run through grit to pick up this magic
Thanks Dan, getting close hopefully. https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/60009/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:25: (color % 1) == 0 && color >= 0 && color <= 0xffffff) { On 2013/04/06 02:51:47, Dan Beam wrote: > On 2013/04/06 02:45:16, Jered wrote: > > On 2013/04/06 00:18:12, Dan Beam wrote: > > > if (!isFinite(color) || color < 0 || color > 0xffffff) > > > return null; > > I'll keep the typeof check for extra sanity if you don't mind. > > > > > > also, what is the % 1 for? > > Colors cannot have fractional parts. > > Math.floor() or Math.round() or assert(color % 1 == 0) or > assert(Math.floor(color) == color); Is this clearer? https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/71029/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:10: (function() { On 2013/04/06 02:51:47, Dan Beam wrote: > On 2013/04/06 02:45:16, Jered wrote: > > On 2013/04/06 00:18:13, Dan Beam wrote: > > > <include src="../path/to/shared/util.js> > > > > I get an error about an unrecognized < token. What's supposed to do this > > including and is it worth the effort to hook it up here? > > files should be run through grit to pick up this magic Ah, I was missing flattenhtml="true". After reading through this code, though, I'd rather not include it for a couple reasons. 1) assert() is the only function we need, and is small and obvious. 2) Including the file has side effects. It binds a click listener on the document to execute a chrome api for file: and about: link clicks. We don't want that behavior for this page. I'm surprised a file called util.js has such a side-effect... Instead, I've just copied the assert function inline here, hopefully that's ok.
super close https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.html (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.html:13: <iframe style="display:none" ^ use hidden instead of style="display:none;" https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:158: function assert(condition, opt_message) { move this to a file and include it from here and util.js https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:755: if (pendingBox && pendingBox.isResponseCurrent(message.data.loaded)) { no curlies https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:27: if (typeof(color) == 'number' && isFinite(color) && typeof color == 'number' (don't use parens), isFinite() checks that something is a number implicitly I'm pretty sure... https://codereview.chromium.org/13375003/diff/98001/chrome/browser/search/sug... File chrome/browser/search/suggestion_source_unittest.cc (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/search/sug... chrome/browser/search/suggestion_source_unittest.cc:92: StartDataRequest("loader.html?origin=chunkybacon"); chunky bacon!? https://codereview.chromium.org/13375003/diff/98001/chrome/common/url_constan... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/13375003/diff/98001/chrome/common/url_constan... chrome/common/url_constants.cc:524: const char kChromeSearchSuggestionURL[] = "chrome-search://suggestion/"; s/URL/Url/
https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.html (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.html:13: <iframe style="display:none" On 2013/04/08 16:46:58, Dan Beam wrote: > ^ use hidden instead of style="display:none;" Done. https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:158: function assert(condition, opt_message) { On 2013/04/08 16:46:58, Dan Beam wrote: > move this to a file and include it from here and util.js Done. https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:755: if (pendingBox && pendingBox.isResponseCurrent(message.data.loaded)) { On 2013/04/08 16:46:58, Dan Beam wrote: > no curlies Done. https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:27: if (typeof(color) == 'number' && isFinite(color) && On 2013/04/08 16:46:58, Dan Beam wrote: > typeof color == 'number' (don't use parens), isFinite() checks that something is > a number implicitly I'm pretty sure... isFinite first converts the object to a number if it is not a number. If it is an object, that will call valueOf. And I think that would be a vector for getting untrusted code to run within this iframe. https://codereview.chromium.org/13375003/diff/98001/chrome/browser/search/sug... File chrome/browser/search/suggestion_source_unittest.cc (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/search/sug... chrome/browser/search/suggestion_source_unittest.cc:92: StartDataRequest("loader.html?origin=chunkybacon"); On 2013/04/08 16:46:58, Dan Beam wrote: > chunky bacon!? :-) http://akuaku.org/images/chunky%20bacon.png https://codereview.chromium.org/13375003/diff/98001/chrome/common/url_constan... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/13375003/diff/98001/chrome/common/url_constan... chrome/common/url_constants.cc:524: const char kChromeSearchSuggestionURL[] = "chrome-search://suggestion/"; On 2013/04/08 16:46:58, Dan Beam wrote: > s/URL/Url/ This way is consistent with most usage in this file (all but two are capitalized).
+sky, -brettw (please feel free to add yourself back if you've started though) Please review renderer/chrome_content_client changes.
Brett is a better reviewer of this code.
On 2013/04/08 23:08:18, sky wrote: > Brett is a better reviewer of this code. Ok, thanks. Brett please let me know if you're able to do this review.
content_renderer_client, chrome_browser.gypi, renderer/resources LGTM https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:1243: *new_url = url.ReplaceComponents(set_origin); I'd just return the new one rather than using the out param, this makes the syntax look much more natural.
Thanks Brett. Sky, would you mind taking a look for the miscellany? chrome/browser/browser_resouces.gyp chrome/common https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:1243: *new_url = url.ReplaceComponents(set_origin); On 2013/04/08 23:16:42, brettw wrote: > I'd just return the new one rather than using the out param, this makes the > syntax look much more natural. Done.
https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:92: if (message.origin != EMBEDDER_ORIGIN) Where does |EMBEDDER_ORIGIN| get set? (Up top it's just a format string.) https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:72: // param is always set by the renderer for requests to this host. We assume renderers are untrustworthy. https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.h (right): https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... chrome/browser/search/suggestion_source.h:44: // Sends Javascript with an &origin parameter interpolated. I'm sorry, I still don't understand why this is safe and won't break anything. https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:1240: // Origin should not include a trailing slash. That is part of the path. Does this mean that GURL.GetOrigin.spec has a bug? https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:1243: *new_url = url.ReplaceComponents(set_origin); > I'd just return the new one rather than using the out param, this makes the > syntax look much more natural. Agreed. https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client_unittest.cc:360: "origin=http://evil.example.com"), result); What about malformed and incomplete origins, and multiple origin parameters?
LGTM
Questions for you Chris to be sure I understand... https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:92: if (message.origin != EMBEDDER_ORIGIN) On 2013/04/09 00:26:15, Chris P. wrote: > Where does |EMBEDDER_ORIGIN| get set? (Up top it's just a format string.) It is interpolated within the URLDataSource which renders this Javascript. See suggestion_source.cc in this CL. https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:72: // param is always set by the renderer for requests to this host. On 2013/04/09 00:26:15, Chris P. wrote: > We assume renderers are untrustworthy. What does that mean in the context of this code? https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.h (right): https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... chrome/browser/search/suggestion_source.h:44: // Sends Javascript with an &origin parameter interpolated. On 2013/04/09 00:26:15, Chris P. wrote: > I'm sorry, I still don't understand why this is safe and won't break anything. Can you be more specific here? Why is it potentially unsafe and what might it break? For example, I'm pretty sure the code to interpolate the value of origin into the Javascript is correct, and that the code in chrome/renderer/content_render_client.cc which inserts it is correct. I'm _not_ sure it's ok to assume that the webframe's top url is actually legit, but that's the origin that a postMessage from the top frame in that renderer is going to have. https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:1240: // Origin should not include a trailing slash. That is part of the path. On 2013/04/09 00:26:15, Chris P. wrote: > Does this mean that GURL.GetOrigin.spec has a bug? No, spec() is intended to stringify URLs, not origins. https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client.cc:1243: *new_url = url.ReplaceComponents(set_origin); On 2013/04/09 00:26:15, Chris P. wrote: > > I'd just return the new one rather than using the out param, this makes the > > syntax look much more natural. > > Agreed. Done. https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client_unittest.cc:360: "origin=http://evil.example.com"), result); On 2013/04/09 00:26:15, Chris P. wrote: > What about malformed and incomplete origins, and multiple origin parameters? Do you mean what if the top_url param is malformed or incomplete, i.e. the webframe URL is invalid? [The entire query string is replaced, but sure, I can add some tests for multiple params too.]
https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:27: if (typeof(color) == 'number' && isFinite(color) && On 2013/04/08 21:43:16, Jered wrote: > On 2013/04/08 16:46:58, Dan Beam wrote: > > typeof color == 'number' (don't use parens), isFinite() checks that something > is > > a number implicitly I'm pretty sure... > > isFinite first converts the object to a number if it is not a number. If it is > an object, that will call valueOf. And I think that would be a vector for > getting untrusted code to run within this iframe. TIL this concern turns out to be unfounded because postMessage makes a structured clone of the message data which doesn't include functions. I can remove the type check if you prefer.
https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/98001/chrome/browser/resources/... chrome/browser/resources/omnibox_result_loader.js:27: if (typeof(color) == 'number' && isFinite(color) && On 2013/04/08 16:46:58, Dan Beam wrote: > typeof color == 'number' (don't use parens), isFinite() checks that something is > a number implicitly I'm pretty sure... Done. https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:72: // param is always set by the renderer for requests to this host. On 2013/04/09 01:18:58, Jered wrote: > On 2013/04/09 00:26:15, Chris P. wrote: > > We assume renderers are untrustworthy. > > What does that mean in the context of this code? I updated the comment. ChromeContentRendererClient is a strange animal, some kind of singleton created from MainDelegate to herd renderers, but I don't think in a render process. Anyway I added some extra validation here. The main goal of plumbing this origin all around is just to ensure that evil.adnetwork.example.com, also iframed on instant.page.example.com and thus in the same Instant render process, can't display suggestion iframes. https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... File chrome/renderer/chrome_content_renderer_client_unittest.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/renderer/chrome_c... chrome/renderer/chrome_content_renderer_client_unittest.cc:360: "origin=http://evil.example.com"), result); On 2013/04/09 00:26:15, Chris P. wrote: > What about malformed and incomplete origins, and multiple origin parameters? Done, I think.
lgtm https://codereview.chromium.org/13375003/diff/98001/chrome/common/url_constan... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/13375003/diff/98001/chrome/common/url_constan... chrome/common/url_constants.cc:524: const char kChromeSearchSuggestionURL[] = "chrome-search://suggestion/"; On 2013/04/08 21:43:16, Jered wrote: > On 2013/04/08 16:46:58, Dan Beam wrote: > > s/URL/Url/ > > This way is consistent with most usage in this file (all but two are > capitalized). This is the old (not endorsed way). Just ignore this now and I'll fix them later. https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css (right): https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css:44: position: absolute; alpha sort https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.html (right): https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.html:14: id="suggestion-loader"></iframe> nit: put hidden last https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:10: <include src="../../../../ui/webui/resources/js/assert.js"></include> nit: drop the </include> https://codereview.chromium.org/13375003/diff/137001/ui/webui/resources/js/ut... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/13375003/diff/137001/ui/webui/resources/js/ut... ui/webui/resources/js/util.js:5: <include src="assert.js"></include> nit: and here, too (no </include>)
https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:72: // param is always set by the renderer for requests to this host. > > We assume renderers are untrustworthy. > > What does that mean in the context of this code? A compromised renderer could set an incorrect/malicious origin, and the browser might make security decisions based on that. Obviously, a compromised origin is already a problem, but we hope that even bad renderers cannot confuse the browser. https://codereview.chromium.org/13375003/diff/137001/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/137001/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:91: std::string response(base::StringPrintf(template_js.as_string().c_str(), This mechanism seems a bit over-powered. Given that |EMBEDDER_ORIGIN| is just "%s", this is really just a string assignment. Is there a simpler, more constrained way to achieve this?
Overall, this CL is large and seems to have lots of security implications, but to be honest I don't fully understand the CL. I'm going to see if I can pull in some more security engineer help (probably jschuh).
On 2013/04/09 19:11:53, Chris P. wrote: > Overall, this CL is large and seems to have lots of security implications, but > to be honest I don't fully understand the CL. I'm going to see if I can pull in > some more security engineer help (probably jschuh). Thanks, I appreciate the thoroughness.
Thanks for the detailed review, Chris (and others)! Chris: I definitely agree that there are a lot of security issues to think through here, but overall I think we can say that this approach is strictly better than the status-quo of using shadow DOM. What do you think of landing this change with your initial review, and you guys can continue pounding on it to see what issues we may have missed? (As you will all be doing anyway in general for Instant Extended in preparation for launch.) I bring it up just because this is a pretty large change and I'd rather land it as early as possible in the release cycle so we can also discover any other issues related to functionality or stability. Thanks! Samarth
https://codereview.chromium.org/13375003/diff/98001/chrome/common/url_constan... File chrome/common/url_constants.cc (right): https://codereview.chromium.org/13375003/diff/98001/chrome/common/url_constan... chrome/common/url_constants.cc:524: const char kChromeSearchSuggestionURL[] = "chrome-search://suggestion/"; On 2013/04/09 03:56:05, Dan Beam wrote: > On 2013/04/08 21:43:16, Jered wrote: > > On 2013/04/08 16:46:58, Dan Beam wrote: > > > s/URL/Url/ > > > > This way is consistent with most usage in this file (all but two are > > capitalized). > > This is the old (not endorsed way). Just ignore this now and I'll fix them > later. Done. https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/108002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:72: // param is always set by the renderer for requests to this host. On 2013/04/09 19:09:55, Chris P. wrote: > > > We assume renderers are untrustworthy. > > > > What does that mean in the context of this code? > > A compromised renderer could set an incorrect/malicious origin, and the browser > might make security decisions based on that. Obviously, a compromised origin is > already a problem, but we hope that even bad renderers cannot confuse the > browser. An alternative approach here would be to check the postMessage origin against a whitelist of allowed origins. These would include *.google.tld$, the local omnibox popup origin, and --instant-url if it is set to enable local developers to see suggestions. This would most easily be formulated as a regular expression passed into the JS, but I'm happy to do this in any way. Would you prefer that approach? Another alternative is to divine from the browser what the Instant URL for the renderer requesting this resource should have been. This is tricky because that state is controlled by preferences which may have since changed, and may reside in some far flung corner of another process somewhere, and so would have to get here asynchronously before this request does. I'm happy to do whatever is best here of course. https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css (right): https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.css:44: position: absolute; On 2013/04/09 03:56:05, Dan Beam wrote: > alpha sort Done. https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.html (right): https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.html:14: id="suggestion-loader"></iframe> On 2013/04/09 03:56:05, Dan Beam wrote: > nit: put hidden last Done. https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/137001/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:10: <include src="../../../../ui/webui/resources/js/assert.js"></include> On 2013/04/09 03:56:05, Dan Beam wrote: > nit: drop the </include> Done. https://codereview.chromium.org/13375003/diff/137001/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/137001/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:91: std::string response(base::StringPrintf(template_js.as_string().c_str(), On 2013/04/09 19:09:55, Chris P. wrote: > This mechanism seems a bit over-powered. Given that |EMBEDDER_ORIGIN| is just > "%s", this is really just a string assignment. Is there a simpler, more > constrained way to achieve this? How about this? https://codereview.chromium.org/13375003/diff/137001/ui/webui/resources/js/ut... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/13375003/diff/137001/ui/webui/resources/js/ut... ui/webui/resources/js/util.js:5: <include src="assert.js"></include> On 2013/04/09 03:56:05, Dan Beam wrote: > nit: and here, too (no </include>) Done.
I'm having some trouble patching this change in. What is the base revision? https://codereview.chromium.org/13375003/diff/157002/ui/webui/resources/js/ut... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/13375003/diff/157002/ui/webui/resources/js/ut... ui/webui/resources/js/util.js:5: <include src="assert.js"> Is this file missing from the CL?
Chris, any new thoughts? Michael, PS23 has a rebase and unfortunately tricky merge (the longer this is pending the harder it's going to get.) https://codereview.chromium.org/13375003/diff/157002/ui/webui/resources/js/ut... File ui/webui/resources/js/util.js (right): https://codereview.chromium.org/13375003/diff/157002/ui/webui/resources/js/ut... ui/webui/resources/js/util.js:5: <include src="assert.js"> On 2013/04/10 18:08:19, melevin wrote: > Is this file missing from the CL? Added in PS22.
Please review. sreeram -> chrome/browser/search jam -> url_data_source palmer -> security Background for url_data_source change: My URLDataSource needs to know some additional information keyed on the net::URLRequest to send the correct request data. Instead of updating 45 callsites to use a really heavyweight net::URLRequest instead of a std::string, I've added another interface to let it rewrite that string to include the extra information. Security: After looking into the net::URLRequest route some more, I ran into a couple of brick walls and decided to go this way instead. Now, the origin is coming entirely from browser state, and is actually a stricter check (of something we expect rather than just a check that something is what it is). Also, I was totally missing a call to InstantIOContext::ShouldProcessRequest(), which meant that the data source wasn't getting the chrome-search:// protection. Sorry. Unfortunately I only noticed this after reading all of the InstantService-related code.
jam: Please also review content/browser/webui/url_data_manager_backend.cc
lgtm https://codereview.chromium.org/13375003/diff/204001/content/public/browser/u... File content/public/browser/url_data_source.cc (right): https://codereview.chromium.org/13375003/diff/204001/content/public/browser/u... content/public/browser/url_data_source.cc:57: std::string* path) const { nit: inline this in the header since clang won't complain in this case (no return type)
Thanks for the quick review. https://codereview.chromium.org/13375003/diff/204001/content/public/browser/u... File content/public/browser/url_data_source.cc (right): https://codereview.chromium.org/13375003/diff/204001/content/public/browser/u... content/public/browser/url_data_source.cc:57: std::string* path) const { On 2013/04/12 16:57:19, jam wrote: > nit: inline this in the header since clang won't complain in this case (no > return type) Done.
PS25 updates the SuggestionSource unittest, apologies as it took a full day to understand all the threading assumptions in this code.
sreeram, palmer, any more thoughts here? I would like to land this soon. On 2013/04/12 21:14:46, Jered wrote: > PS25 updates the SuggestionSource unittest, apologies as it took a full day to > understand all the threading assumptions in this code.
On 2013/04/15 14:21:46, Jered wrote: > sreeram, palmer, any more thoughts here? I would like to land this soon. Excitingly, I am revising this again to use yet another way of getting the origin, because the site URL really does not. I will ping when that is done. > > On 2013/04/12 21:14:46, Jered wrote: > > PS25 updates the SuggestionSource unittest, apologies as it took a full day to > > understand all the threading assumptions in this code.
ok PTAL Broadly, what I've been struggling with here is a reasonable way to get the expected origin for the postMessage call without patching it up through the renderer. I thought I could track the Instant URL per process along with the Instant process ids where they're registered, but that turned out not to work correctly (the "site URL" is actually a key for bucketing processes, and plus there can be multiple Instant pages per process). So instead, this change looks up the Instant WebContents by mapping the process and render view id from the RequestContextInfo with RenderProcessHost, and then uses the origin of its current URL. That seems to do what we want. Thanks for your patience, this CL has been highly educational. On 2013/04/15 23:31:50, Jered wrote: > On 2013/04/15 14:21:46, Jered wrote: > > sreeram, palmer, any more thoughts here? I would like to land this soon. > > Excitingly, I am revising this again to use yet another way of getting the > origin, because the site URL really does not. I will ping when that is done. > > > > > On 2013/04/12 21:14:46, Jered wrote: > > > PS25 updates the SuggestionSource unittest, apologies as it took a full day > to > > > understand all the threading assumptions in this code.
Sorry for the delay. Still reviewing... https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/resource... File chrome/renderer/resources/extensions/searchbox_api.js (right): https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/resource... chrome/renderer/resources/extensions/searchbox_api.js:240: }; Nit: Add a blank line after this method, to make it clear that the restriction is only for this method and not for the following ones. https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:104: // starts with those prefixes. Shouldn't the comment be "... unless it starts with |user_input|", since that's what the method does? Though it does seem to me that the method should be modified too, since it probably doesn't do what we want (see next comment). https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:108: if (StartsWith(*url, trimmed_user_input, true)) So, if I type "h", and the URL match is "http://www.hotmail.com/", this won't strip anything. Right? Don't we want to strip the "http://www."? Similarly for an input like "w". https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:121: // trailing "/". Same issue as the other method. The comment talks about the |query| matching some prefix, whereas the method uses |query| as the prefix to match against the URL. https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:127: if (EndsWith(*url, trimmed_user_input, true)) This stumped me a bit until I read the following lines (where you strip the "/"). I think it would be clearer if you merged the two blocks into one: // Remove a trailing slash, but only if the user hasn't typed it explicitly. if (EndsWith(*url, "/") && !EndsWith(*url, user_input)) url->erase(...);
https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:1235: WebKit::WebFrame* webframe = WebKit::WebFrame::frameForCurrentContext(); I'm confident you've thought this through, but just to be on the same page, it isn't possible for the search engine page to spoof this, right? Say with something like: iframe = document.getElementById('loader').contentWindow; hax0red = chrome.searchBox.getSuggestionData.call(iframe);
https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:1235: WebKit::WebFrame* webframe = WebKit::WebFrame::frameForCurrentContext(); On 2013/04/16 19:27:23, sreeram wrote: > I'm confident you've thought this through, No, I am not an expert on WebKit security, and need absolutely all the attention I can get on this CL from people who are. > but just to be on the same page, it > isn't possible for the search engine page to spoof this, right? Say with > something like: > > iframe = document.getElementById('loader').contentWindow; > hax0red = chrome.searchBox.getSuggestionData.call(iframe); I truly hope that does not affect the frameForCurrentContext(). I will try this.
https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:1235: WebKit::WebFrame* webframe = WebKit::WebFrame::frameForCurrentContext(); On 2013/04/16 19:32:47, Jered wrote: > On 2013/04/16 19:27:23, sreeram wrote: > > I'm confident you've thought this through, > No, I am not an expert on WebKit security, and need absolutely all the attention > I can get on this CL from people who are. > > > but just to be on the same page, it > > isn't possible for the search engine page to spoof this, right? Say with > > something like: > > > > iframe = document.getElementById('loader').contentWindow; > > hax0red = chrome.searchBox.getSuggestionData.call(iframe); > I truly hope that does not affect the frameForCurrentContext(). I will try this. I verified that the Javascript call you described does not pass the origin check below. It still has the origin of the search engine page.
https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/resource... File chrome/renderer/resources/extensions/searchbox_api.js (right): https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/resource... chrome/renderer/resources/extensions/searchbox_api.js:240: }; On 2013/04/16 18:23:16, sreeram wrote: > Nit: Add a blank line after this method, to make it clear that the restriction > is only for this method and not for the following ones. Done. https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:104: // starts with those prefixes. On 2013/04/16 18:23:16, sreeram wrote: > Shouldn't the comment be "... unless it starts with |user_input|", since that's > what the method does? Though it does seem to me that the method should be > modified too, since it probably doesn't do what we want (see next comment). I just moved all this code from searchbox.cc to here, because it was only being called from here. I will ask samarth@ for help on its intended behavior and try to address your comments about it.
https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:104: // starts with those prefixes. On 2013/04/16 20:12:49, Jered wrote: > On 2013/04/16 18:23:16, sreeram wrote: > > Shouldn't the comment be "... unless it starts with |user_input|", since > that's > > what the method does? Though it does seem to me that the method should be > > modified too, since it probably doesn't do what we want (see next comment). > > I just moved all this code from searchbox.cc to here, because it was only being > called from here. I will ask samarth@ for help on its intended behavior and try > to address your comments about it. Let's not try to fix everything in this patch. Sreeram, I filed crbug.com/232047 to track these comments and I'll send you a patch later this week addressing these.
https://codereview.chromium.org/13375003/diff/225001/chrome/browser/resources... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/225001/chrome/browser/resources... chrome/browser/resources/omnibox_result_loader.js:100: var iframe = window.parent.frames[iframeId]; Just FYI: I spent a couple of days trying to crack this, by having the search page override its frames array or otherwise cause the JS here to get an object that wasn't actually an iframe. None of that worked. iframes are really rock solid! https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:80: // this here because GetOrigin() needs to run on the UI thread. Does it make sense to add DCHECK(IsOnThread(...)) to all the methods here? https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:172: *origin = contents->GetURL().GetOrigin().spec(); Is it possible for this to be spoofed? Say the search page navigates to "chrome-search://suggestion/..." (that's correct, it sets location.href to navigate). But before doing that, it installs an unload handler that takes a while to execute. Now for some reason (maybe the unload handler adds the loader iframe), we get here. I assume GetURL() would return the "chrome-search://" URL. No? (Since the active entry is the pending entry.) I've now lost track of what I was trying to do, but perhaps it's now possible for the search page to spoof something?
https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:172: *origin = contents->GetURL().GetOrigin().spec(); On 2013/04/16 21:23:31, sreeram wrote: > Is it possible for this to be spoofed? Say the search page navigates to > "chrome-search://suggestion/..." (that's correct, it sets location.href to > navigate). But before doing that, it installs an unload handler that takes a > while to execute. Now for some reason (maybe the unload handler adds the loader > iframe), we get here. I assume GetURL() would return the "chrome-search://" URL. > No? (Since the active entry is the pending entry.) I've now lost track of what I > was trying to do, but perhaps it's now possible for the search page to spoof > something? Don't waste too much time on this. For the page to be able to do anything at all, the unload handler must yield, at which point, the page will probably have been considered to be unloaded. So, I don't think there's any path that leads from an unload handler to a successful attack. I mentioned it in case it sparks any interesting lines of attack/consideration in your mind.
LGTM
Thanks for reviewing. https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:172: *origin = contents->GetURL().GetOrigin().spec(); On 2013/04/16 21:23:31, sreeram wrote: > Is it possible for this to be spoofed? Say the search page navigates to > "chrome-search://suggestion/..." (that's correct, it sets location.href to > navigate). But before doing that, it installs an unload handler that takes a > while to execute. Now for some reason (maybe the unload handler adds the loader > iframe), we get here. I assume GetURL() would return the "chrome-search://" URL. > No? (Since the active entry is the pending entry.) I've now lost track of what I > was trying to do, but perhaps it's now possible for the search page to spoof > something? I think WebContents is getting its URL from the navigation controller. I'll play around with unload and some of the html5 history hooks, but my intuition is that this will happen before the url is updated and won't get an attacker anything.
https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:80: // this here because GetOrigin() needs to run on the UI thread. On 2013/04/16 21:23:31, sreeram wrote: > Does it make sense to add DCHECK(IsOnThread(...)) to all the methods here? I'd really prefer not to. I have seen this idiom a couple of places in chrome and am completely baffled by it. It makes code needlessly hard to unit test and buys almost nothing for a private method in a class with ~7 methods. https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:172: *origin = contents->GetURL().GetOrigin().spec(); On 2013/04/16 21:28:19, sreeram wrote: > On 2013/04/16 21:23:31, sreeram wrote: > > Is it possible for this to be spoofed? Say the search page navigates to > > "chrome-search://suggestion/..." (that's correct, it sets location.href to > > navigate). But before doing that, it installs an unload handler that takes a > > while to execute. Now for some reason (maybe the unload handler adds the > loader > > iframe), we get here. I assume GetURL() would return the "chrome-search://" > URL. > > No? (Since the active entry is the pending entry.) I've now lost track of what > I > > was trying to do, but perhaps it's now possible for the search page to spoof > > something? > > Don't waste too much time on this. For the page to be able to do anything at > all, the unload handler must yield, at which point, the page will probably have > been considered to be unloaded. So, I don't think there's any path that leads > from an unload handler to a successful attack. I mentioned it in case it sparks > any interesting lines of attack/consideration in your mind. tl;dr There doesn't seem to be a viable attack here. The biggest problem would be if you could get your code to run with SecurityOrigin chrome-search://suggestion, since then you could call the privileged api or read result iframes. That doesn't seem possible; I checked that onunload() still has the correct SecurityOrigin. It would also be mildly problematic if you could get any page to be able to show suggestions by getting a loader iframe to accept a non-Instant origin. I tried setting location.href="http://www.example.com" and then adding a loader iframe in onunload(), but it still has the expected origin of the Instant page, not the pending page, and I can't do anything with the newly created iframe, because postMessage is async and I can't cancel the navigation at that point. I also tried hooking on to onbeforeunload. Surprisingly, Instant doesn't block the modal dialog, and I can see the iframe element I created. But it is still set up with the origin of the Instant page.
ptal palmer; PS34 is built on 14039004 which is pending with approvals. On 2013/04/18 18:11:48, Jered wrote: > https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... > File chrome/browser/search/suggestion_source.cc (right): > > https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... > chrome/browser/search/suggestion_source.cc:80: // this here because GetOrigin() > needs to run on the UI thread. > On 2013/04/16 21:23:31, sreeram wrote: > > Does it make sense to add DCHECK(IsOnThread(...)) to all the methods here? > > I'd really prefer not to. > > I have seen this idiom a couple of places in chrome and am completely baffled by > it. It makes code needlessly hard to unit test and buys almost nothing for a > private method in a class with ~7 methods. > > https://codereview.chromium.org/13375003/diff/225001/chrome/browser/search/su... > chrome/browser/search/suggestion_source.cc:172: *origin = > contents->GetURL().GetOrigin().spec(); > On 2013/04/16 21:28:19, sreeram wrote: > > On 2013/04/16 21:23:31, sreeram wrote: > > > Is it possible for this to be spoofed? Say the search page navigates to > > > "chrome-search://suggestion/..." (that's correct, it sets location.href to > > > navigate). But before doing that, it installs an unload handler that takes a > > > while to execute. Now for some reason (maybe the unload handler adds the > > loader > > > iframe), we get here. I assume GetURL() would return the "chrome-search://" > > URL. > > > No? (Since the active entry is the pending entry.) I've now lost track of > what > > I > > > was trying to do, but perhaps it's now possible for the search page to spoof > > > something? > > > > Don't waste too much time on this. For the page to be able to do anything at > > all, the unload handler must yield, at which point, the page will probably > have > > been considered to be unloaded. So, I don't think there's any path that leads > > from an unload handler to a successful attack. I mentioned it in case it > sparks > > any interesting lines of attack/consideration in your mind. > > tl;dr There doesn't seem to be a viable attack here. > > The biggest problem would be if you could get your code to run with > SecurityOrigin chrome-search://suggestion, since then you could call the > privileged api or read result iframes. That doesn't seem possible; I checked > that onunload() still has the correct SecurityOrigin. > > It would also be mildly problematic if you could get any page to be able to show > suggestions by getting a loader iframe to accept a non-Instant origin. I tried > setting location.href="http://www.example.com" and then adding a loader iframe > in onunload(), but it still has the expected origin of the Instant page, not the > pending page, and I can't do anything with the newly created iframe, because > postMessage is async and I can't cancel the navigation at that point. > > I also tried hooking on to onbeforeunload. Surprisingly, Instant doesn't block > the modal dialog, and I can see the iframe element I created. But it is still > set up with the origin of the Instant page.
Can you rebase past r195323?
I expect that to take a few full days next week. :-) On Apr 19, 2013 6:13 PM, <melevin@chromium.org> wrote: > Can you rebase past r195323? > > https://codereview.chromium.**org/13375003/<https://codereview.chromium.org/1... >
Getting very close! just some tweaks and questions. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:524: this.suggestions_[this.selectedIndex_].restrcitedId); typo: restrictedId https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:529: * Returns the restricted id of the iframe clicked. This is redundant; you have a @return comment. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:531: * @return {?number} The restricted id clicked or null if none. "...ID of the iframe that was clicked, or null if there was none." https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:542: * Called when the user hovers on the specified iframe. ... for the purpose of... Updating hoveredIndex_, and redrawing. Explain the side-effects. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:557: * Called when the user unhovers the specified iframe. Same thing: say why. It seems like this function could just be a straight-forward call to clearHover? Basically, if |iframeId| does not exist, then the caller is in error, and there should be some kind of debug warning. Then once you've verified that your callers are correct, you could get rid of the check? https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:679: if (!!inputValue && suggestions.length) { Do you get the same result without the !! ? https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/omnibox_result_loader.js:13: * This string literal must be in double quotes for proper escaping. I'm not a JS expert; why is that? https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:56: SendJSWithOrigin(IDR_OMNIBOX_RESULT_LOADER_JS, render_process_id, Definitely want to use { ... } when the statement bodies spam multiple lines. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:91: ReplaceFirstSubstringAfterOffset(&response, 0, "{{ORIGIN}}", origin); Inside the JS code, an origin check will happen. Perhaps we should do a check here in C++, so that incorrect origins have even less of a chance of affecting JS execution. Is that possible? Do we know enough at this stage to perform a check? https://codereview.chromium.org/13375003/diff/150002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/theme_source.cc (left): https://codereview.chromium.org/13375003/diff/150002/chrome/browser/ui/webui/... chrome/browser/ui/webui/theme_source.cc:72: (uncached_path == kNewIncognitoTabCSSPath && is_incognito)); Are we sure it's safe to remove this DCHECK? https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/resource... File chrome/renderer/resources/extensions/searchbox_api.js (right): https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/resource... chrome/renderer/resources/extensions/searchbox_api.js:132: escapeHTML(result.contents) + '</span>'; This function is not safe for escaping anything but HTML. Are we sure result.contents and result.description are always only plain HTML, no JS? https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/searchbo... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:1244: if (!(url.SchemeIs(chrome::kChromeSearchScheme) && Ahh, I guess this is the early, C++-level check I was asking about in a previous comment. Great. Is this re-phrasing more clear? : if (!url.SchemeIs(chrome::kChromeSearchScheme) || url.host != chrome::kChromeSearchSuggestionHost) { return undefined... } https://codereview.chromium.org/13375003/diff/150002/content/browser/webui/we... File content/browser/webui/web_ui_data_source_unittest.cc (right): https://codereview.chromium.org/13375003/diff/150002/content/browser/webui/we... content/browser/webui/web_ui_data_source_unittest.cc:62: 0, 0, Elsewhere, you used -1 to indicate no particular IDs. Which is right?
PTAL. Note there are two patches included in this patch, 14150011 to fix chromium on chromeos which is my test plafrom and 14039004 to add the int params I need to SuggestionSource. The latter patch is on the way in and the former patch is not immediately relevant and you can ignore it (content_renderer.gypi). Thanks! https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js (right): https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:524: this.suggestions_[this.selectedIndex_].restrcitedId); On 2013/04/22 19:38:37, Chromium Palmer wrote: > typo: restrictedId Done. Good catch! https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:529: * Returns the restricted id of the iframe clicked. On 2013/04/22 19:38:37, Chromium Palmer wrote: > This is redundant; you have a @return comment. Done. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:531: * @return {?number} The restricted id clicked or null if none. On 2013/04/22 19:38:37, Chromium Palmer wrote: > "...ID of the iframe that was clicked, or null if there was none." Done. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:542: * Called when the user hovers on the specified iframe. On 2013/04/22 19:38:37, Chromium Palmer wrote: > ... for the purpose of... Updating hoveredIndex_, and redrawing. Explain the > side-effects. Done. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:557: * Called when the user unhovers the specified iframe. On 2013/04/22 19:38:37, Chromium Palmer wrote: > Same thing: say why. It seems like this function could just be a > straight-forward call to clearHover? Basically, if |iframeId| does not exist, > then the caller is in error, and there should be some kind of debug warning. > Then once you've verified that your callers are correct, you could get rid of > the check? I'd prefer to keep the check. This method is called in response to a mouseout event, and we only want that event to control hover for the iframe it came from. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:679: if (!!inputValue && suggestions.length) { On 2013/04/22 19:38:37, Chromium Palmer wrote: > Do you get the same result without the !! ? You get the same truthiness, but not the same result (you'd get '' here instead of false for instance). https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... File chrome/browser/resources/omnibox_result_loader.js (right): https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources... chrome/browser/resources/omnibox_result_loader.js:13: * This string literal must be in double quotes for proper escaping. On 2013/04/22 19:38:37, Chromium Palmer wrote: > I'm not a JS expert; why is that? Because I'm using a JSON escaping function in Chrome, since it seemed like the most robust way to escape a JS string in Chrome C++ code. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su... File chrome/browser/search/suggestion_source.cc (right): https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:56: SendJSWithOrigin(IDR_OMNIBOX_RESULT_LOADER_JS, render_process_id, On 2013/04/22 19:38:37, Chromium Palmer wrote: > Definitely want to use { ... } when the statement bodies spam multiple lines. Done. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su... chrome/browser/search/suggestion_source.cc:91: ReplaceFirstSubstringAfterOffset(&response, 0, "{{ORIGIN}}", origin); On 2013/04/22 19:38:37, Chromium Palmer wrote: > Inside the JS code, an origin check will happen. Perhaps we should do a check > here in C++, so that incorrect origins have even less of a chance of affecting > JS execution. Is that possible? Do we know enough at this stage to perform a > check? No, we do not know the origin for the postMessage() call until we receive the postMessage() call. https://codereview.chromium.org/13375003/diff/150002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/theme_source.cc (left): https://codereview.chromium.org/13375003/diff/150002/chrome/browser/ui/webui/... chrome/browser/ui/webui/theme_source.cc:72: (uncached_path == kNewIncognitoTabCSSPath && is_incognito)); On 2013/04/22 19:38:37, Chromium Palmer wrote: > Are we sure it's safe to remove this DCHECK? Yes. The owners agreed this was ok in https://codereview.chromium.org/14039004/ of which this is a patch. https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/resource... File chrome/renderer/resources/extensions/searchbox_api.js (right): https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/resource... chrome/renderer/resources/extensions/searchbox_api.js:132: escapeHTML(result.contents) + '</span>'; On 2013/04/22 19:38:37, Chromium Palmer wrote: > This function is not safe for escaping anything but HTML. Are we sure > result.contents and result.description are always only plain HTML, no JS? This code is not used by my change (and in fact the goal of my change is to enable us to delete it). That said, result.contents and result.description are not interpolated as Javascript. https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/searchbo... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/searchbo... chrome/renderer/searchbox/searchbox_extension.cc:1244: if (!(url.SchemeIs(chrome::kChromeSearchScheme) && On 2013/04/22 19:38:37, Chromium Palmer wrote: > Ahh, I guess this is the early, C++-level check I was asking about in a previous > comment. Great. Not quite. This check asserts that callers to the searchBox.getSuggestionData() method which will reveal super secret information must have the origin chrome-search://suggestion. That is different to the other code location where we're checking the expected origin of a postMessage() call is the Instant page (i.e. the google.com page which is allowed to show suggestions). > > Is this re-phrasing more clear? : > > if (!url.SchemeIs(chrome::kChromeSearchScheme) || > url.host != chrome::kChromeSearchSuggestionHost) { > return undefined... > } They're almost identical to me but I spent too many years designing circuits. Done. https://codereview.chromium.org/13375003/diff/150002/content/browser/webui/we... File content/browser/webui/web_ui_data_source_unittest.cc (right): https://codereview.chromium.org/13375003/diff/150002/content/browser/webui/we... content/browser/webui/web_ui_data_source_unittest.cc:62: 0, 0, On 2013/04/22 19:38:37, Chromium Palmer wrote: > Elsewhere, you used -1 to indicate no particular IDs. Which is right? It's a no-op in all these test cases because it's ignored. I just haphazardly picked 0. Would -1 be clearer?
Note that all the popup code has now moved to local_ntp.{html,css.js} files.
On 2013/04/22 23:02:57, Jered wrote:
> PTAL.
>
> Note there are two patches included in this patch, 14150011 to fix chromium on
> chromeos which is my test plafrom and 14039004 to add the int params I need to
> SuggestionSource. The latter patch is on the way in and the former patch is
not
> immediately relevant and you can ignore it (content_renderer.gypi).
>
> Thanks!
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> File chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js
> (right):
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:524:
> this.suggestions_[this.selectedIndex_].restrcitedId);
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > typo: restrictedId
> Done. Good catch!
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:529: *
> Returns the restricted id of the iframe clicked.
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > This is redundant; you have a @return comment.
>
> Done.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:531: *
> @return {?number} The restricted id clicked or null if none.
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > "...ID of the iframe that was clicked, or null if there was none."
>
> Done.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:542: *
> Called when the user hovers on the specified iframe.
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > ... for the purpose of... Updating hoveredIndex_, and redrawing. Explain
the
> > side-effects.
>
> Done.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:557: *
> Called when the user unhovers the specified iframe.
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > Same thing: say why. It seems like this function could just be a
> > straight-forward call to clearHover? Basically, if |iframeId| does not
exist,
> > then the caller is in error, and there should be some kind of debug warning.
> > Then once you've verified that your callers are correct, you could get rid
of
> > the check?
>
> I'd prefer to keep the check. This method is called in response to a mouseout
> event, and we only want that event to control hover for the iframe it came
from.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> chrome/browser/resources/local_omnibox_popup/local_omnibox_popup.js:679: if
> (!!inputValue && suggestions.length) {
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > Do you get the same result without the !! ?
>
> You get the same truthiness, but not the same result (you'd get '' here
instead
> of false for instance).
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> File chrome/browser/resources/omnibox_result_loader.js (right):
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/resources...
> chrome/browser/resources/omnibox_result_loader.js:13: * This string literal
must
> be in double quotes for proper escaping.
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > I'm not a JS expert; why is that?
>
> Because I'm using a JSON escaping function in Chrome, since it seemed like the
> most robust way to escape a JS string in Chrome C++ code.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su...
> File chrome/browser/search/suggestion_source.cc (right):
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su...
> chrome/browser/search/suggestion_source.cc:56:
> SendJSWithOrigin(IDR_OMNIBOX_RESULT_LOADER_JS, render_process_id,
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > Definitely want to use { ... } when the statement bodies spam multiple
lines.
>
> Done.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/search/su...
> chrome/browser/search/suggestion_source.cc:91:
> ReplaceFirstSubstringAfterOffset(&response, 0, "{{ORIGIN}}", origin);
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > Inside the JS code, an origin check will happen. Perhaps we should do a
check
> > here in C++, so that incorrect origins have even less of a chance of
affecting
> > JS execution. Is that possible? Do we know enough at this stage to perform a
> > check?
>
> No, we do not know the origin for the postMessage() call until we receive the
> postMessage() call.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/theme_source.cc (left):
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/theme_source.cc:72: (uncached_path ==
> kNewIncognitoTabCSSPath && is_incognito));
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > Are we sure it's safe to remove this DCHECK?
>
> Yes. The owners agreed this was ok in
https://codereview.chromium.org/14039004/
> of which this is a patch.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/resource...
> File chrome/renderer/resources/extensions/searchbox_api.js (right):
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/resource...
> chrome/renderer/resources/extensions/searchbox_api.js:132:
> escapeHTML(result.contents) + '</span>';
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > This function is not safe for escaping anything but HTML. Are we sure
> > result.contents and result.description are always only plain HTML, no JS?
>
> This code is not used by my change (and in fact the goal of my change is to
> enable us to delete it). That said, result.contents and result.description are
> not interpolated as Javascript.
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/searchbo...
> File chrome/renderer/searchbox/searchbox_extension.cc (right):
>
>
https://codereview.chromium.org/13375003/diff/150002/chrome/renderer/searchbo...
> chrome/renderer/searchbox/searchbox_extension.cc:1244: if
> (!(url.SchemeIs(chrome::kChromeSearchScheme) &&
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > Ahh, I guess this is the early, C++-level check I was asking about in a
> previous
> > comment. Great.
> Not quite. This check asserts that callers to the
searchBox.getSuggestionData()
> method which will reveal super secret information must have the origin
> chrome-search://suggestion.
>
> That is different to the other code location where we're checking the expected
> origin of a postMessage() call is the Instant page (i.e. the http://google.com
page
> which is allowed to show suggestions).
>
> >
> > Is this re-phrasing more clear? :
> >
> > if (!url.SchemeIs(chrome::kChromeSearchScheme) ||
> > url.host != chrome::kChromeSearchSuggestionHost) {
> > return undefined...
> > }
>
> They're almost identical to me but I spent too many years designing circuits.
> Done.
>
>
https://codereview.chromium.org/13375003/diff/150002/content/browser/webui/we...
> File content/browser/webui/web_ui_data_source_unittest.cc (right):
>
>
https://codereview.chromium.org/13375003/diff/150002/content/browser/webui/we...
> content/browser/webui/web_ui_data_source_unittest.cc:62: 0, 0,
> On 2013/04/22 19:38:37, Chromium Palmer wrote:
> > Elsewhere, you used -1 to indicate no particular IDs. Which is right?
>
> It's a no-op in all these test cases because it's ignored. I just haphazardly
> picked 0. Would -1 be clearer?
LGTM https://codereview.chromium.org/13375003/diff/150002/content/browser/webui/we... File content/browser/webui/web_ui_data_source_unittest.cc (right): https://codereview.chromium.org/13375003/diff/150002/content/browser/webui/we... content/browser/webui/web_ui_data_source_unittest.cc:62: 0, 0, > It's a no-op in all these test cases because it's ignored. I just haphazardly > picked 0. Would -1 be clearer? I only asked because you used -1 elsewhere, and it seems better to have just one "this value is meaningless" value. It's not a huge deal.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jered@chromium.org/13375003/271001
Presubmit check for 13375003-271001 failed and returned exit status 1. INFO:root:Found 32 file(s). Running presubmit commit checks ... Running /b/commit-queue/workdir/chromium/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py ** Presubmit Messages ** See the JavaScript style guide at http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript and if you have any feedback about the JavaScript PRESUBMIT check, contact tbreisacher@chromium.org or dbeam@chromium.org ** Presubmit Warnings ** Found JavaScript style violations in chrome/browser/resources/omnibox_result.js: line 16: E0131: Single-quoted string preferred over double-quoted string. var EMBEDDER_ORIGIN = "{{ORIGIN}}"; ^ Found JavaScript style violations in chrome/browser/resources/omnibox_result_loader.js: line 17: E0131: Single-quoted string preferred over double-quoted string. var EMBEDDER_ORIGIN = "{{ORIGIN}}"; ^ Presubmit checks took 5.0s to calculate.
Thanks, commit-bot. That was helpful. There are two lines of comments explaining why this exception is necessary. I guess I will spend an hour digging through Python to try to figure out how to shut this off. On 2013/04/23 23:15:58, I haz the power (commit-bot) wrote: > Presubmit check for 13375003-271001 failed and returned exit status 1. > > INFO:root:Found 32 file(s). > > Running presubmit commit checks ... > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/browser/resources/PRESUBMIT.py > > ** Presubmit Messages ** > See the JavaScript style guide at > http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript > and if you have any feedback about the JavaScript PRESUBMIT check, contact > mailto:tbreisacher@chromium.org or mailto:dbeam@chromium.org > > ** Presubmit Warnings ** > Found JavaScript style violations in chrome/browser/resources/omnibox_result.js: > line 16: E0131: Single-quoted string preferred over double-quoted string. > var EMBEDDER_ORIGIN = "{{ORIGIN}}"; > ^ > > Found JavaScript style violations in > chrome/browser/resources/omnibox_result_loader.js: > line 17: E0131: Single-quoted string preferred over double-quoted string. > var EMBEDDER_ORIGIN = "{{ORIGIN}}"; > ^ > > Presubmit checks took 5.0s to calculate.
See crbug.com/222642 for an example how to add an exception. Until then, you can use git cl try to run the tryjobs and then dcommit the change.
On 2013/04/23 23:21:16, samarth wrote: > See crbug.com/222642 for an example how to add an exception. Until then, you > can use git cl try to run the tryjobs and then dcommit the change. Is there a //nolint? Most linters have something like that right?
I know that's not possible in CSS because the Chromium CSS linter strips out comments. There may be a nolint directive you can use in javascript.
On 2013/04/23 23:23:08, Jered wrote: > On 2013/04/23 23:21:16, samarth wrote: > > See crbug.com/222642 for an example how to add an exception. Until then, you > > can use git cl try to run the tryjobs and then dcommit the change. > > Is there a //nolint? Most linters have something like that right? nope :(
On 2013/04/23 23:26:12, Dan Beam wrote: > On 2013/04/23 23:23:08, Jered wrote: > > On 2013/04/23 23:21:16, samarth wrote: > > > See crbug.com/222642 for an example how to add an exception. Until then, > you > > > can use git cl try to run the tryjobs and then dcommit the change. > > > > Is there a //nolint? Most linters have something like that right? > > nope :( Hrm yeah jscompiler has @suppress, and gjslint actually parses it, but only to emit a lint warning if it has a value that isn't in a frozenset (it doesn't seem to actually do any suppressing). That's just wow. Ok thanks.
On 2013/04/23 23:42:33, Jered wrote: > On 2013/04/23 23:26:12, Dan Beam wrote: > > On 2013/04/23 23:23:08, Jered wrote: > > > On 2013/04/23 23:21:16, samarth wrote: > > > > See crbug.com/222642 for an example how to add an exception. Until then, > > you > > > > can use git cl try to run the tryjobs and then dcommit the change. > > > > > > Is there a //nolint? Most linters have something like that right? > > > > nope :( > > Hrm yeah jscompiler has @suppress, and gjslint actually parses it, but only to > emit a lint warning if it has a value that isn't in a frozenset (it doesn't seem > to actually do any suppressing). That's just wow. Ok thanks. I'm reasonably confident the remaining try failures are not due to this CL and will dcommit.
Message was sent while issue was closed.
Committed patchset #39 manually as r196184 (presubmit successful). |
