[about:omnibox]: create encapsulating MojomProxy class responsible for sending requests to and receiving responses from the omnibox page handler c++ code. [chromium/src : master]
manuk hovanesian (Gerrit)
about:omnibox, a refactor to move c++ communicating code into a single class
To view, visit change 1252324. To unsubscribe, or for help writing mail filters, visit settings.
Tommy Li (Gerrit)
dpapad: Can I send this review to you? Manuk is creating a JavaScript abstraction for Mojo communication with JavaScript.
Since you are actively involved in guiding the direction of similar things, I think you would be the best person to influence the direction of this.
Demetrios Papadopoulos (Gerrit)
Patch Set 5:
dpapad: Can I send this review to you? Manuk is creating a JavaScript abstraction for Mojo communication with JavaScript.
Since you are actively involved in guiding the direction of similar things, I think you would be the best person to influence the direction of this.
Left some comments. Having said that, as the Mojo discussion (for Settings) is ongoing, and I am examining various CLs that propose Mojo usage in WebUI, I don't have any concrete patterns to suggest at this point.
Given that this is a non-user-facing WebUI page, I think it's OK to go with what works for this specific case, and re-evaluate once more concrete patterns have emerged.
Also (optional) nit: The "about:" prefix is pretty old AFAIK, and predates "chrome://". It is still preserved for legacy reasons, but I don't think we need to keep using that terminology.
Suggesting to prefix similar CLs with "Omnibox WebUI: ....."
4 comments:
File chrome/browser/resources/omnibox/omnibox.js:
Patch Set #6, Line 402:
class MojomProxy {FWIW, I find the nested "Proxy" objects a bit confusing. What is the difference between "browser" and "mojo" terminology? I guess what I am saying is that don't understand why "mojo" wraps "browser", or whether it should be the reverse.
Could we come up with some better names? Maybe the following?
rename MojomProxy to BrowserProxy
rename |browserProxy| to pagehandlePtr_, which at least matches the equivalent class?Patch Set #6, Line 404:
this.browserProxy = new mojom.OmniboxPageHandlerPtr;Can you add @type declarations for this? Otherwise the compiler does not know the types.
Patch Set #6, Line 410:
this.binding = new mojo.Binding(mojom.OmniboxPage, this, mojo.makeRequest(client));Same here.
Patch Set #6, Line 451:
mojomProxy = new MojomProxy();Should this be moved in initialize? Then you can simply change this line to
document.addEventListener('DOMContentLoaded', initialize);
To view, visit change 1252324. To unsubscribe, or for help writing mail filters, visit settings.
manuk hovanesian (Gerrit)
manuk hovanesian uploaded patch set #8 to this change.
[chrome://omnibox]: create encapsulating MojomProxy class responsible for sending requests to and receiving responses from the omnibox page handler c++ code.
Change-Id: I5aaa363a6614c8120c05cc4923c032a0d79a6157
---
M chrome/browser/resources/omnibox/omnibox.js
1 file changed, 60 insertions(+), 61 deletions(-)
To view, visit change 1252324. To unsubscribe, or for help writing mail filters, visit settings.
manuk hovanesian (Gerrit)
applied the suggested renames and typdefs.
1 comment:
File chrome/browser/resources/omnibox/omnibox.js:
Should this be moved in initialize? Then you can simply change this line to […]
i will rename `initialize` to `initializeEventHandlers` as that's what it's really doing. let me know if u still would like me to replace the anonymous function with a named `initialize` function.
To view, visit change 1252324. To unsubscribe, or for help writing mail filters, visit settings.
Demetrios Papadopoulos (Gerrit)
Patch Set 8:
(1 comment)
applied the suggested renames and typdefs.
Could you mark previously addressed comments as "Done". This makes reviewing a bit easier.
Also, couple more comments/questions about BrowserProxy class design.
Currently it is accessing things from the outside scope, like refreshNewResult(), as well as querying the DOM to get the text value to be sent to C++. Have you considered making it a pure wrapper of the Mojo API, without coupling it with anything else?
3 comments:
File chrome/browser/resources/omnibox/omnibox.js:
Patch Set #9, Line 377:
addResultToOutput(browserProxy.progressiveAutocompleteResults[browserProxy.progressiveAutocompleteResults.length - 1]);80 char limit? Was this the output of clang-format?
@private {!mojom.OmniboxPageHandlerPtr}
/**
* @type {!mojo.Binding}
*/Nit: Can fit in one line?
/** @type {!mojo.Binding} */Should this be private as well?
To view, visit change 1252324. To unsubscribe, or for help writing mail filters, visit settings.
manuk hovanesian (Gerrit)
Patch Set 9:
(3 comments)
Patch Set 8:
(1 comment)
applied the suggested renames and typdefs.
Could you mark previously addressed comments as "Done". This makes reviewing a bit easier.
Also, couple more comments/questions about BrowserProxy class design.
Currently it is accessing things from the outside scope, like refreshNewResult(), as well as querying the DOM to get the text value to be sent to C++. Have you considered making it a pure wrapper of the Mojo API, without coupling it with anything else?
that's part of the next* cl to encapsulate DOM access into its own class; I thought it was worth splitting into 2 parts.
*i might reorder the cls after this rebase, but it'll be in the next 1-5 cl's
7 comments:
File chrome/browser/resources/omnibox/omnibox.js:
Patch Set #6, Line 402:
function clearOutput() {FWIW, I find the nested "Proxy" objects a bit confusing. […]
Done
Patch Set #6, Line 404:
omniboxDebugText.removeChild(omniboxDebugText.firstChild);
Can you add @type declarations for this? Otherwise the compiler does not know the types.
Done
Patch Set #6, Line 410:
this.pagehandlePtr_ = new mojom.OmniboxPageHandlerPtr;Same here.
Done
i will rename `initialize` to `initializeEventHandlers` as that's what it's really doing. […]
Done
File chrome/browser/resources/omnibox/omnibox.js:
Patch Set #9, Line 377:
else if (browserProxy.progressiveAutocompleteResults.length)
80 char limit? Was this the output of clang-format?
Done
@private {!mojom. […]
Done
mojo.makeRequest(this.pagehandlePtr_).handle);
let client = new mojom.OmniboxPagePtr;
//Nit: Can fit in one line? […]
Done
To view, visit change 1252324. To unsubscribe, or for help writing mail filters, visit settings.
Demetrios Papadopoulos (Gerrit)
Patch Set 10:
(7 comments)
Patch Set 9:
(3 comments)
Patch Set 8:
(1 comment)
applied the suggested renames and typdefs.
Could you mark previously addressed comments as "Done". This makes reviewing a bit easier.
Also, couple more comments/questions about BrowserProxy class design.
Currently it is accessing things from the outside scope, like refreshNewResult(), as well as querying the DOM to get the text value to be sent to C++. Have you considered making it a pure wrapper of the Mojo API, without coupling it with anything else?
that's part of the next* cl to encapsulate DOM access into its own class; I thought it was worth splitting into 2 parts.
*i might reorder the cls after this rebase, but it'll be in the next 1-5 cl's
Are you planning to fire events from BrowserProxy, that the UI code can listen for? I think that would be a reasonable way to decouple BrowserProxy from the code that actually updates the UI.
Note that there is a helper cr.ui.EventTarget class that you can use to fire events on at https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/event_target.js.
There is also a native EventTarget class, which would be preferable to use (https://bugs.chromium.org/p/chromium/issues/detail?id=854268), but Closure compiler unfortunately does not understand it yet.
Finally, the CL description needs to be updated after your lastest changes. I also suggest to try to keep the 1st sentence of your description shorter, and put any extra information after a blank line, in a follow up sentence.
Patch set 11:Code-Review +1
manuk hovanesian (Gerrit)
Patch Set 11: Code-Review+1
Patch Set 10:
(7 comments)
Patch Set 9:
(3 comments)
Patch Set 8:
(1 comment)
applied the suggested renames and typdefs.
Could you mark previously addressed comments as "Done". This makes reviewing a bit easier.
Also, couple more comments/questions about BrowserProxy class design.
Currently it is accessing things from the outside scope, like refreshNewResult(), as well as querying the DOM to get the text value to be sent to C++. Have you considered making it a pure wrapper of the Mojo API, without coupling it with anything else?
that's part of the next* cl to encapsulate DOM access into its own class; I thought it was worth splitting into 2 parts.
*i might reorder the cls after this rebase, but it'll be in the next 1-5 cl'sAre you planning to fire events from BrowserProxy, that the UI code can listen for? I think that would be a reasonable way to decouple BrowserProxy from the code that actually updates the UI.
Note that there is a helper cr.ui.EventTarget class that you can use to fire events on at https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/event_target.js.
There is also a native EventTarget class, which would be preferable to use (https://bugs.chromium.org/p/chromium/issues/detail?id=854268), but Closure compiler unfortunately does not understand it yet.
Finally, the CL description needs to be updated after your lastest changes. I also suggest to try to keep the 1st sentence of your description shorter, and put any extra information after a blank line, in a follow up sentence.
For js -> c++, the dom values are passed to the browserProxy as paramters. For c++ -> js, the browserProxy calls `add` when receiving a new set of results, and `updateImageData` when receiving async image data for a previous result. I'll look into the event firing if it makes it cleaner or more isolated. Below is what I mean (not yet rebased with this cl's updates)
```
class MojomProxy {
constructor() {
this.browserProxy = new mojom.OmniboxPageHandlerPtr;
Mojo.bindInterface(mojom.OmniboxPageHandler.name, mojo.makeRequest(this.browserProxy).handle);
let client = new mojom.OmniboxPagePtr;
this.binding = new mojo.Binding(mojom.OmniboxPage, this, mojo.makeRequest(client));
this.browserProxy.setClientPage(client);
}
makeRequest(resetAutocompleteController, inputString, cursorPosition, preventInlineAutocomplete, preferKeyword, pageClassification) {
this.lastCursorPosition = cursorPosition;
this.browserProxy.startOmniboxQuery(resetAutocompleteController, inputString, cursorPosition, preventInlineAutocomplete, preferKeyword, pageClassification);
} handleNewAutocompleteResult(response) { // triggered from c++
outputData.add(response, this.lastCursorPosition);
} handleNewImageData(imageUrl, imageData) { // triggered from c++
outputData.updateImageData(imageUrl, imageData);
}
```