Closed Bug 1210410 Opened 9 years ago Closed 9 years ago

Implement search messages for the Remote New Tab page

Categories

(Firefox :: New Tab Page, defect, P1)

Product:
Firefox
Component:
New Tab Page
Version:
40 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Iteration:
44.3 - Nov 2
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: oyiptong, Assigned: ursula)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 10 obsolete files)

MozReview Request: Bug 1210410 - Implement search messages for the Remote New Tab page r?mconley
40 bytes, text/x-review-board-request
Details
Refactored ContentSearch.jsm
20.90 KB, patch
ursula
: feedback+
Details | Diff | Splinter Review
MozReview Request: Bug 1210410 Implement search messages for the Remote New Tab
58 bytes, text/x-review-board-request
oyiptong
: review+
Details
This tracks the creation of new messages for search to work in the remote new tab page
No longer blocks: landing-remote-newtab
Assignee: nobody → ursulasarracini
Currently the list of messages necessary to implement the new one-off design for search looks like this: NewTab:Search NewTab:GetState NewTab:GetStrings NewTab:GetSuggestions NewTab:RemoveFormHistoryEntry NewTab:ManageEngines NewTab:SetCurrentEngine
Bug 1210410 - Implement search messages for the Remote New Tab page r?emtwo
Attachment #8671583 - Flags: review?(msamuel)
Attachment #8671582 - Attachment is obsolete: true
Comment on attachment 8671583 [details] MozReview Request: Bug 1210410 - Implement search messages for the Remote New Tab page r?emtwo https://reviewboard.mozilla.org/r/21609/#review19467 This looks good overall, just a couple of comments. Also, are we aiming for 100% test coverage like we are for remote-newtab? I'm ok with these tests for now but I think there are some untested parts (e.g. the msg handling in RemoteAboutNewTab.jsm) ::: browser/components/newtab/RemoteAboutNewTab.jsm:235 (Diff revision 1) > * may have changed, then proceed with the page update. We'll need to update this comment to include that we're listening to browser-search-engine-modified as well. ::: browser/components/newtab/RemoteAboutNewTab.jsm:248 (Diff revision 1) > + } else if (aTopic === "browser-search-engine-modified" && aData === "engine-current") { Are we listening for this notification? We need to add it in the _addObservers() function (and remove observers) ::: browser/components/newtab/tests/browser/browser_SearchProvider.js:27 (Diff revision 1) > + // state returns promise when eventually returns a state object nit: `and eventually`? ::: browser/components/newtab/tests/browser/browser_SearchProvider.js:75 (Diff revision 1) > + // adding an entry to the form history will a 'formhistory-add', so we need to wait for will what? ::: browser/components/newtab/tests/browser/browser_SearchProvider.js:91 (Diff revision 1) > + win.gBrowser.tabs[1].linkedBrowser.removeEventListener("pageshow", pageShow); can we do a check to confirm the length of win.gBrowser.tabs before we index it? ::: browser/components/newtab/tests/browser/browser_SearchProvider.js:115 (Diff revision 1) > + is(formHistory[0], searchData.searchString, "the search string has been added to form history"); Let's check for length of formHistory before indexing it here.
Attachment #8671583 - Flags: review?(msamuel)
https://reviewboard.mozilla.org/r/21609/#review19467 > Are we listening for this notification? We need to add it in the _addObservers() function (and remove observers) some screwiness happened during a rebase and this was lost along the way...also lost along the way was the "ns:PrefChanged" observer which was needed when aData === "browser.search.hiddenOneOffs" so that a message can be sent down to update the current state. Will add both of these back
https://reviewboard.mozilla.org/r/21607/#review19495 Hey oyiptong, for future reference, if you've got a stack of patches that haven't yet landed on m-c, and only one patch that you want to post to MozReview, you can do that with -c. Example: hg push -c [SHA FOR X] review Even if X is based on patches that have not yet landed.
awesome, thank you!
Attached patch bug_1210410.patch (obsolete) — — Splinter Review
Attachment #8671583 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff25645a05855ef09a359b4d46d4701976003ff Bug 1210410 - Implement search messages for the Remote New Tab page r=emtwo r=oyiptong
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7cafc955419d - so far at least, it seems to be doing fine on other platforms, but on Windows browser_SearchProvider.js times out like https://treeherder.mozilla.org/logviewer.html#?job_id=16508492&repo=mozilla-inbound
Attached patch bug_1210410.v3.patch (obsolete) — — Splinter Review
Attachment #8673943 - Attachment is obsolete: true
Comment on attachment 8671582 [details] MozReview Request: Bug 1210410 - Implement search messages for the Remote New Tab page r?mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21607/diff/1-2/
Attachment #8671582 - Attachment description: MozReview Request: other changes reviewed elsewhere → MozReview Request: Bug 1210410 - Implement search messages for the Remote New Tab page r?mconley
Attachment #8671582 - Attachment is obsolete: false
Attachment #8671582 - Flags: review?(mconley)
Attachment #8682561 - Attachment is obsolete: true
Comment on attachment 8671582 [details] MozReview Request: Bug 1210410 - Implement search messages for the Remote New Tab page r?mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/21607/diff/2-3/
Let me just summarize the conversations that oyiptong and I have been having outside of Bugzilla about this bug: The first thing that jumped out about this patch series was that SearchProvider.jsm seemed to implement a lot of the same functionality as ContentSearch.jsm, and I wasn't sure why were doing that. So there are a few things to be aware of here: 1) SearchProvider.jsm makes use of RemotePageManager.jsm instead of the event communications model that ContentSearch.jsm + tab-content.js uses. 2) The Content Services team is hoping to add this support to 45 (Nightly) and 44 (Aurora), as this is an explicit goal that they've set. 3) This work is not meant to ship to end users enabled by default (see bug 1221728 for particulars), and effort is underway to switch about:remote-newtab to using a custom WebIDL-based interface that is exposed only to the content loaded via about:remote-newtab. 4) The patch in this bug, and the other RemotePageManager work is going to be ifdef'd out for any non-Nightly and non-Aurora channels. When (3) is ready, all of the RemotePageManager work about:remote-newtab work is going to get torn out - however, it is unlikely to be ready for another release or two. So to sum, this patch is part of a temporary solution for getting about:remote-newtab into the tree, but in a state that will never ship to end users preffed on. oyiptong, is this accurate? If so, can you please comment on the exact reasoning behind the goal of getting this work into 44? I think we talked about it, but I can't find it in my notes.
Flags: needinfo?(oyiptong)
Comment on attachment 8671582 [details] MozReview Request: Bug 1210410 - Implement search messages for the Remote New Tab page r?mconley https://reviewboard.mozilla.org/r/21607/#review22765 So my primary concern here is that SearchProvider.jsm seems to re-implement a lot of the same logic that ContentSearch.jsm provides. Assuming we want to go the about:remote-newtab -> RemoteAboutNewTab.jsm directly for search (thereby bypassing the tab-content.js ContentSearchMediator...) then what I'd suggest is exposing the methods that you need on ContentSearch.jsm as an API that RemoteAboutNewTab.jsm can consume, as opposed to re-implementing that API in SearchProvider.jsm.
Attachment #8671582 - Flags: review?(mconley)
As mconley has stated, the current patch is a temporary solution. After the work in bug 1218998, we'll replace most things from the current patches. About the first issue: the messaging code in RemoteAboutNewTab.jsm goes away with the IDL. The second issue is about SearchProvider. there is a concern that the code in SearchProvider may prove to be a maintenance problem, since the code is almost identical to ContentSearch.jsm. It is better exposing/making public the functionality we need in ContentSearch.jsm and use those instead.
Flags: needinfo?(oyiptong)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #26) > Comment on attachment 8671582 [details] > MozReview Request: Bug 1210410 - Implement search messages for the Remote > New Tab page r?mconley > > https://reviewboard.mozilla.org/r/21607/#review22765 > > So my primary concern here is that SearchProvider.jsm seems to re-implement > a lot of the same logic that ContentSearch.jsm provides. > > Assuming we want to go the about:remote-newtab -> RemoteAboutNewTab.jsm > directly for search (thereby bypassing the tab-content.js > ContentSearchMediator...) then what I'd suggest is exposing the methods that > you need on ContentSearch.jsm as an API that RemoteAboutNewTab.jsm can > consume, as opposed to re-implementing that API in SearchProvider.jsm. So, we tried working with ContentSearch.jsm, and had to give up. I don't say this lightly, and with little hyperbole: it is possibly the most over-engineered and worst designed API I've seen in a long time (take a look, it's just awful... it doens't have any methods and instead replaces everything unnecessarily with message passing! I honestly don't know how that thing made it through code review). I would urge us to instead deprecate ContentSearch.jsm ASAP in favor of SearchProvider.jsm. SearchProvider.jsm provides the same functionality in a way that is sane, approachable, and maintainable.
Flags: needinfo?(mconley)
(In reply to Marcos Caceres [:marcosc] from comment #28) > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #26) > > Comment on attachment 8671582 [details] > > MozReview Request: Bug 1210410 - Implement search messages for the Remote > > New Tab page r?mconley > > > > https://reviewboard.mozilla.org/r/21607/#review22765 > > > > So my primary concern here is that SearchProvider.jsm seems to re-implement > > a lot of the same logic that ContentSearch.jsm provides. > > > > Assuming we want to go the about:remote-newtab -> RemoteAboutNewTab.jsm > > directly for search (thereby bypassing the tab-content.js > > ContentSearchMediator...) then what I'd suggest is exposing the methods that > > you need on ContentSearch.jsm as an API that RemoteAboutNewTab.jsm can > > consume, as opposed to re-implementing that API in SearchProvider.jsm. > > So, we tried working with ContentSearch.jsm, and had to give up. I don't say > this lightly, and with little hyperbole: it is possibly the most > over-engineered and worst designed API I've seen in a long time (take a > look, it's just awful... it doens't have any methods and instead replaces > everything unnecessarily with message passing! I honestly don't know how > that thing made it through code review). > > I would urge us to instead deprecate ContentSearch.jsm ASAP in favor of > SearchProvider.jsm. SearchProvider.jsm provides the same functionality in a > way that is sane, approachable, and maintainable. Hm. I think Florian worked on ContentSearch - he might have some input on this. Perhaps the common code between ContentSearch and SearchProvider could be extracted out, and ContentSearch could use it? The duplication is just killing me here. Florian, what do you think?
Flags: needinfo?(mconley) → needinfo?(florian)
Now that Marcos shakes his fists in the air, I remember some grunts of exasperation.
(In reply to Olivier Yiptong [:oyiptong] from comment #30) > Now that Marcos shakes his fists in the air, I remember some grunts of > exasperation. Apologies, I know my tone is harsh - but I believe it's warranted in this case. ContentSearch seriously needs to die in a fire. I would be willing to work on replacing ContentSearch with SearchProvider throughout Gecko after we finish about:newtab.
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #29) > Hm. I think Florian worked on ContentSearch - he might have some input on > this. Perhaps the common code between ContentSearch and SearchProvider could > be extracted out, and ContentSearch could use it? The duplication is just > killing me here. > > Florian, what do you think? I haven't worked on ContentSearch.jsm (I only touched the file twice in trivial ways). The person you really want to talk to is Drew (on PTO until November 25th unfortunately). Felipe who reviewed the initial version of the file may be able to answer some questions. To be honest, I find the tone of comment 28 and comment 31 inappropriate for bugzilla. The only technical point there is "ContentSearch.jsm uses message passing as an API", the rest is venting. From a very quick look, I found the current ContentSearch.jsm file more approachable (or at least better documented) than the proposed SearchProvider.jsm replacement. I share Mike's concern about code duplication, but don't see any obvious reason why ContentSearch.jsm can't be adapted.
Flags: needinfo?(florian)
Alright, since adw is on PTO, I'm hoping to get felipe's feedback on this too. felipe: There's code in this patch that duplicates ContentSearch.jsm's capabilities. Marcos and oyiptong found some deficiencies in the ContentSearch APIs which resulted in them writing SearchProvider.jsm. Marcos is also suggesting we replace all usage of ContentSearch with SearchProvider through the rest of the browser code. Feedback on that?
Flags: needinfo?(felipc)
I had a quick chat with Marcos about this. A few things come to mind after taking a look at the code in ContentSearch.jsm. At first, mconley correctly observed that there was quite a large amount of duplication in logic. And that is true. However, there are a couple of deficiencies in the implementation: * ContentSearch assumes certain behavior in-content. It was meant for specific implementations of about:newtab and about:home * ContentSearch is opinionated as to how it functions: it sends messages directly to its target. i.e. https://dxr.mozilla.org/mozilla-central/source/browser/modules/ContentSearch.jsm#156 Now, this won't work for us. First, we'd like to stop using messages for about:newtab. Second, as we move move functionality in the unprivileged (and remote) page, we are sending less processed data to the page. For instance, instead of return merged grid data or tiles, we are sending the top 100 frecent sites and then let the page decide how it's going to arrange tiles. As such, we'd like to have more control on what ContentSearch provides, and we certainly don't want messages. What Marcos proposes, in a rather colorful language, is that we rewrite ContentSearch as SearchProvider, that will provide the same functionality, with different assumptions.
I think we're wanting the same thing here - basically, the de-duplication and centralization of the logic that lets searching work from unprivileged content. So here's what I suggest: 1) Whatever we end up with, it should exist under browser/modules, and not browser/components/newtab, since this will be used for multiple content pages 2) It should be able to speak both the language that ContentSearch <-> tab-content.js uses (for about:home and the current about:newtab), but also expose the methods that RemoteAboutNewTab needs to do what it's doing. Certainly there will be overlap between the functions that RemoteAboutNewTab needs and the tab-content consumers need. I suggest these functions should be extracted out into methods in ContentSearch that can be used directly by RemoteAboutNewTab, and also used by the functions within ContentSearch that will send messages to tab-content. It's clear that you've found ContentSearch difficult to work with, and this is a golden opportunity to make it easier to work with, which doesn't involve having two different search-for-content modules running in parallel. Does that sound like a good plan?
Flags: needinfo?(mcaceres)
I'm all for improving code. If you think a new module is better suited here, go for it. I actually don't mind some code duplication for the transition, as long as it's cleaned up afterwards. But I wouldn't want to commit replacing all other users of ContentSearch with something newer without letting Drew take a look, but that doesn't need to block getting the new remote tab to work. Just make sure it doesn't regress any feature of the non-remote tab (during the transition) or about:home. I'm having a hard time understanding how SearchProvider is clearly superior to ContentSearch (they look very similar to me), but I won't go further in that conversation. (In reply to Olivier Yiptong [:oyiptong] from comment #34) > For instance, instead of return merged grid data or tiles, we are sending > the top 100 frecent sites and then let the page decide how it's going to > arrange tiles. I don't understand this. ContentSearch has nothing to do with tiles. It is a module to provide suggestions-as-you-type for the search box in about:home and about:new-tab. > > As such, we'd like to have more control on what ContentSearch provides, and > we certainly don't want messages. Ultimately you want to get data from one process (the Firefox UI) to some web content in a different process. Messages will be involved. The patch as posted here simply moves the messaging responsibility from the ContentSearch/SearchProvider module to RemoteAboutNewTab. I see that you intend to remove them in bug 1218998, but what about e10s? How is that gonna work with multiple processes?
Flags: needinfo?(felipc)
(In reply to :Felipe Gomes from comment #36) > Ultimately you want to get data from one process (the Firefox UI) to some > web content in a different process. Messages will be involved. > > The patch as posted here simply moves the messaging responsibility from the > ContentSearch/SearchProvider module to RemoteAboutNewTab. I see that you > intend to remove them in bug 1218998, but what about e10s? How is that gonna > work with multiple processes? Indeed, the idea would be to still have the ability to send the messages for e10s/about:home and the current about:newtab, but to also have access to the raw data that gets sent as messages.
(In reply to :Felipe Gomes from comment #36) > I'm having a hard time understanding how SearchProvider is clearly superior > to ContentSearch (they look very similar to me), but I won't go further in > that conversation. IMHO: the superiority comes from making it easier to use and test. SearchProvider uses straight methods that return promises, instead of, IMHO, super-confusing message passing + callbacks: The message passing adds a bunch of cognitive overhead making really hard to work with (hence the page-long wall of text that ones needs to read at the top of the file to actually be able to use the API: that alone should have rang alarm-bells that something is seriously wrong with the design of the ContentSearch API). (In reply to Mike Conley (:mconley) - Needinfo me! from comment #35) > I think we're wanting the same thing here - basically, the de-duplication > and centralization of the logic that lets searching work from unprivileged > content. > > So here's what I suggest: > > 1) Whatever we end up with, it should exist under browser/modules, and not > browser/components/newtab, since this will be used for multiple content pages Agree. > 2) It should be able to speak both the language that ContentSearch <-> > tab-content.js uses (for about:home and the current about:newtab), but also > expose the methods that RemoteAboutNewTab needs to do what it's doing. Agree. It might be possible to merge the two to make ContextSearch sane to use. > Certainly there will be overlap between the functions that RemoteAboutNewTab > needs and the tab-content consumers need. I suggest these functions should > be extracted out into methods in ContentSearch that can be used directly by > RemoteAboutNewTab, and also used by the functions within ContentSearch that > will send messages to tab-content. Agree. It might be possible and desirable to do this. > It's clear that you've found ContentSearch difficult to work with, and this > is a golden opportunity to make it easier to work with, which doesn't > involve having two different search-for-content modules running in parallel. > > Does that sound like a good plan? Sounds like a plan. I'll look at doing this as part of implementing SearchProvider as a WebIDL interface exposed on remote about:newtab. Basically, I'll wrap ContextSearch in an WebIDL interface and and try to add the methods to ContentSearch.
Flags: needinfo?(mcaceres)
Assignee: ursulasarracini → mcaceres
Started work on this...
How is search working in the React based newtab?
Flags: needinfo?(ursulasarracini)
No longer blocks: privileged-decisioning
Priority: -- → P1
The search stuff with react is making use of the redux actions and dispatching pattern. There's 3 major 'parts' to the current code in terms of search: 1. Components: 1.1 Search.js renders the search box, it's the react "view". It communicates with SearchActions.js to collect search suggestions by making calls (dispatching actions) like 'getSuggestions()'. 1.2 SearchMagic.js handles the rendering of the drop down one-off search suggestions, it's also a react "view". It knows the current engine, the list of all engines, the suggestions, the search strings, performs the search, and manages the engines. 2. Actions: SearchActions.js dispatches actions, for example "RECEIVE_SEARCH_SUGGESTIONS" or "REQUEST_SEARCH_SUGGESTIONS" when a Web API call has been made (which comes from Search.js or SearchMagic.js), for example 'getSuggestions()'. 2. Reducers: SearchReducer.js is the module used for updating state based on the redux actions that were dispatched. For example it waits for the action "RECEIVE_SEARCH_SUGGESTIONS", and then the body of the action gets set to the list of search suggestion strings (if any). Each 'state' is a bunch of properties (isLoading, currentEngine, SearchString, suggestions etc..) which gets updated when the action is received. Basically it goes like this: Search.js AND SearchMagic.js are the react views and they trigger actions based on user interactions -> SearchActions.js filters what that call to an action was, and dispatches an actual action with a unique name -> SearchReducer.js listens for the dispatched actions and updates the state of the search component. kate hudson set up the search stuff with react/redux so I'm sure she can provide some more detail on each part.
Flags: needinfo?(ursulasarracini)
Update: So now that we're using WebChannel.jsm, the search stuff in content will need to send and receive messages from chrome. There's been lots of debate about whether to use ContentSearch.jsm, or our own SearchProvider.jsm, which has a lot of duplicated code from ContentSearch.jsm. We're going to hook into the existing functionality from ContentSearch.jsm (since about:home also uses it), but refactor it so we don't have to use their message passing framework, extract only the stuff that we need, and use our own NewTabWebChannel.jsm to package it up and communicate to content that way.
Assignee: mcaceres → ursulasarracini
Prior to me going on sick leave, I had done a bunch of work on ContentSearch.jsm. I'll attach as a patch. Changes: * added inbound message handlers for "GetCurrentEngineDetails", "GetEngineDetails", "GetVisibleEngines". * added outbound message: "CurrentEngineDetails", "EngineDetails", "FormHistoryEntryAdded", "FormHistoryEntryRemoved", "VisibleEngines". * Added _getEngineDetailsByName method. * Cleaned up "get searchSuggestionUIStrings()" method - refactored for loop to use map instead. * Cleaned up destroy function - to not return value based on the result of an assignment (makes code linter happy... code quality people!). * Fixed nits found by JSHint (== instead of ===) + a added a few ES6-isms. * JSCS styled the code. * cleaned up _processEventQueue() a little bit. * added _onMessageGetState() * added _onMessageGetCurrentEngineDetails() * added _onMessageGetVisibleEngines() * added _onMessageGetEngineDetails() * added _visibleEnginesDetails() * Cleaned up _onMessageGetStrings * Dropped returning redundant promise from _onMessageManageEngines() and _onMessageAddFormHistoryEntry() * Slightly refactored _onMessageGetSuggestions() * updated some of the code documentation. * import and use real URL object for getting the origin of the preconnectOrigin. * _msgArgs() gained knowledge of IDs (by passing in the message itself)... making ContentSearch.jsm compatible with PromiseMessage.jsm * Made _currentStateObj sync and a getter instead of a function. It was unnecessarily async. The icon data is not returned as a data: URL. * droppped _arrayBufferFromDataURI() - it's redundant if you have a data URL.
Attachment #8715071 - Flags: feedback?(ursulasarracini)
s/The icon data is not returned as a data: URL/The icon data is *now* returned as a data: URL
Comment on attachment 8715071 [details] [diff] [review] Refactored ContentSearch.jsm Thanks for this marcos! Looks great, I'll be using it :)
Attachment #8715071 - Flags: feedback?(ursulasarracini) → feedback+
Attachment #8743634 - Flags: review?(oyiptong)
Attachment #8743634 - Flags: review?(mcaceres)
Attachment #8743634 - Flags: review?(mcaceres) → review+
Comment on attachment 8743634 [details] MozReview Request: Bug 1210410: Implement search messages for the Remote New Tab page https://reviewboard.mozilla.org/r/47961/#review44771 Couple of little things, but generally all good :) ::: browser/components/newtab/NewTabMessages.jsm:67 (Diff revision 1) > outFrecent: "RECEIVE_FRECENT", > outPlacesChange: "RECEIVE_PLACES_CHANGE", > }, > + search: { > + inSearch: { > + UIStrings: "REQUEST_UISTRINGS", nit: whitespace ::: browser/components/newtab/NewTabMessages.jsm:109 (Diff revision 1) > // Return to the originator the top frecent links > PlacesProvider.links.getLinks().then(links => { > NewTabWebChannel.send(ACTIONS.links.outFrecent, links, target); > }); > break; > + case ACTIONS.search.inSearch.UIStrings: It might just be me, but this seems like a lot of indirection for what are just string constants... why not just have "REQUEST_UISTRINGS" or "SEARCH_IN_REQUEST_UISTRINGS". ::: browser/components/newtab/NewTabMessages.jsm:121 (Diff revision 1) > + Task.spawn(function*() { > + try { > + let {engineName, searchString} = data; > + let suggestions = yield NewTabSearchProvider.search.getSuggestions(engineName, searchString, target); > + NewTabWebChannel.send(ACTIONS.search.outSearch.suggestions, suggestions, target); > + } catch (e) { As this catch block is constantly repeated for all but one action... maybe you can instead just write for each one: ``` let promise; switch(...){ case what.ever: promise = Task.spawn(function*(){ //don't bother with try catch here... }) } promise.catch(e => Cu.report(e) ); ``` And just get any sync cases to return Promise.resove(); ::: browser/components/newtab/NewTabMessages.jsm:156 (Diff revision 1) > + Cu.reportError(e); > + } > + }); > + break; > + case ACTIONS.search.inSearch.cycleEngine: > + NewTabSearchProvider.search.cycleEngine(data); Bug here: you forgot to either `yield` or `.then()` here. ::: browser/components/newtab/NewTabMessages.jsm:172 (Diff revision 1) > > /* > + * Broadcast current engine has changed to all open newtab pages > + */ > + _handleCurrentEngineChange(name, value) { > + if (name === CURRENT_ENGINE) { Nit: this one is a bit confusing... I would expected a `if(x !== y)`, then broacast. I'm not saying it's wrong, just the code seems weird/backwards. ::: browser/components/newtab/NewTabSearchProvider.jsm:23 (Diff revision 1) > +XPCOMUtils.defineLazyGetter(this, "EventEmitter", function() { > + const {EventEmitter} = Cu.import("resource://devtools/shared/event-emitter.js", {}); > + return EventEmitter; > +}); > + > +let SearchProvider = function SearchProvider() { Nit: Maybe just ```JS function SearchProvider(){ EventEmitter.decorate(this); } ``` instead of assigning a variable? ::: browser/components/newtab/NewTabSearchProvider.jsm:30 (Diff revision 1) > +}; > + > +SearchProvider.prototype = { > + > + observe(subject, topic, data) { // jshint unused:false > + if (topic === CURRENT_ENGINE && data === "engine-current") { If you are using "data" as the "switch", maybe use a switch statement here. ::: browser/components/newtab/NewTabSearchProvider.jsm:35 (Diff revision 1) > + if (topic === CURRENT_ENGINE && data === "engine-current") { > + Task.spawn(function* () { > + let state = yield ContentSearch.currentStateObj(true); > + let engine = state.currentEngine; > + this.emit(CURRENT_ENGINE, engine); > + }.bind(this)); Nit: you might want to .catch() the promise that is spawned... or try/catch inside the spanwed task, so the potentially rejected promise doesn't get lost. ::: browser/components/newtab/NewTabSearchProvider.jsm:40 (Diff revision 1) > + }.bind(this)); > + } else if (data === "engine-default") { > + // engine-default is always sent with engine-current and isn't > + // relevant to content searches. > + return; > + } Nit: move else up to matching } ::: browser/components/newtab/NewTabSearchProvider.jsm:81 (Diff revision 1) > + > + removeFormHistory(target, suggestion) { > + ContentSearch.removeFormHistoryEntry({target: target.browser}, suggestion); > + }, > + > + performSearch(target, searchData) { Seems you only case about target.browser, so maybe destructure it out? function({browser}, searchData){ } ::: browser/components/newtab/NewTabSearchProvider.jsm:89 (Diff revision 1) > + yield ContentSearch.addFormHistoryEntry({target: target.browser}, searchData.searchString); > + }.bind(this)); > + }, > + > + manageEngines(browser) { > + let browserWin = browser.ownerDocument.defaultView; nit: const ::: browser/components/newtab/NewTabSearchProvider.jsm:110 (Diff revision 1) > + return suggestions; > + }.bind(this)); > + }, > +}; > + > +const gSearch = new SearchProvider(); Not sure why you need gSearch if it's not exported? ::: browser/components/newtab/NewTabSearchProvider.jsm:112 (Diff revision 1) > + }, > +}; > + > +const gSearch = new SearchProvider(); > + > +let NewTabSearchProvider = { probably want this to be a const ::: browser/modules/ContentSearch.jsm:365 (Diff revision 1) > + return; > + } > + > + let event = this._eventQueue.shift(); > + > + this._currentEventPromise = Task.spawn(function* () { I'm confused, when is `_currentEventPromise` first set? That is, how is it set before `_processEventQueue` is called? ::: browser/modules/ContentSearch.jsm:385 (Diff revision 1) > - let { previousFormHistoryResult } = browserData; > - for (let i = 0; i < previousFormHistoryResult.matchCount; i++) { > - if (previousFormHistoryResult.getValueAt(i) == entry) { > - previousFormHistoryResult.removeValueAt(i, true); > - break; > + if (this._currentSuggestion && this._currentSuggestion.target === msg.target) { > + this._currentSuggestion.controller.stop(); > + cancelled = true; > + } > + // cancel queued suggestion requests > + for (let i = 0; i < this._eventQueue.length; i++) { why u no?: ``` this._eventQueueu .map(item => item.data) .filter(data => msg.target === data.target .filter({data} => && data.type === "GetSuggestions") .somethingSomething ... I can't work out what it's doing any more ``` The current algorithm is really confusing. Particularly, cancelled can switch from true/false after each loop around... if this is true, then you might as well just run the loop backwards. I don't know. I hate for loops, they hurt my brain.
https://reviewboard.mozilla.org/r/47961/#review44973 ::: browser/modules/ContentSearch.jsm:365 (Diff revision 1) > + return; > + } > + > + let event = this._eventQueue.shift(); > + > + this._currentEventPromise = Task.spawn(function* () { this is old content search code which i don't know how it works and didn't wrote. dont look at me :( ::: browser/modules/ContentSearch.jsm:385 (Diff revision 1) > - let { previousFormHistoryResult } = browserData; > - for (let i = 0; i < previousFormHistoryResult.matchCount; i++) { > - if (previousFormHistoryResult.getValueAt(i) == entry) { > - previousFormHistoryResult.removeValueAt(i, true); > - break; > + if (this._currentSuggestion && this._currentSuggestion.target === msg.target) { > + this._currentSuggestion.controller.stop(); > + cancelled = true; > + } > + // cancel queued suggestion requests > + for (let i = 0; i < this._eventQueue.length; i++) { more old content search code :'( i'm just gonna leave it for now tho because i also don't understand it
https://reviewboard.mozilla.org/r/47961/#review44973 > more old content search code :'( > > i'm just gonna leave it for now tho because i also don't understand it Perhaps adw or nhnt11 could be consulted here.
https://reviewboard.mozilla.org/r/47961/#review44771 > It might just be me, but this seems like a lot of indirection for what are just string constants... why not just have "REQUEST_UISTRINGS" or "SEARCH_IN_REQUEST_UISTRINGS". If I'm not mistaken it's to keep the action names consistent with how React/Redux handle the incoming/outgoing messages. I think it will help if the format for the message naming changes in the future, so that we won't have to go in and change it in every switch case, or in any other part of the code where they might be used > As this catch block is constantly repeated for all but one action... maybe you can instead just write for each one: > > ``` > let promise; > > switch(...){ > case what.ever: > promise = Task.spawn(function*(){ > //don't bother with try catch here... > }) > } > > promise.catch(e => Cu.report(e) ); > ``` > > And just get any sync cases to return Promise.resove(); Hmm, I like the idea of reducing the try-catches but I'm worried that in the sync cases (even the ones that haven't to do with search) I'll have to set ```promise = Promise.resolve()``` in each one, which I don't really like: let promise; switch(...){ case what.ever.async: promise = Task.spawn(function*(){ // don't bother with try catch here... }); break; case what.ever.sync: // do some sync stuff promise = Promise.resolve(); break; } promise.catch(e => Cu.report(e) ); From my understanding if the async stuff never hits and only the sync stuff hits, then the promise will be undefined, so we would have to do let promise; switch(...){ case what.ever.async: promise = Task.spawn(function*(){ //don't bother with try catch here... }); break; case what.ever.sync: // do some sync stuff return Promise.resolve(); } promise.catch(e => Cu.report(e) ); Which I don't know if we necessarily want to return out of the handler alltogether at that point. It also means we have some cases returning a promise we won't use in the future, and some cases just breaking from the switch. Am I understanding what you meant correctly? > Nit: this one is a bit confusing... I would expected a `if(x !== y)`, then broacast. I'm not saying it's wrong, just the code seems weird/backwards. Looking at this now I think it's redundant... This will never receive a different name, since the event emitter in NewTabSearchProvider only emits CURRENT_ENGINE. I'm pretty sure I can remove this if statement alltogether.
Comment on attachment 8743634 [details] MozReview Request: Bug 1210410: Implement search messages for the Remote New Tab page https://reviewboard.mozilla.org/r/47961/#review45273 ::: browser/components/newtab/tests/browser/browser_newtabsearchprovider.js:3 (Diff revision 1) > +"use strict"; > + > +/* global XPCOMUtils, NewTabSearchProvider, run_next_test, ok, is, Services */ Is there a reason this is a browser test and not an xpcshell test? Also: we need tests to test the messages
Attachment #8743634 - Flags: review?(oyiptong)
Attachment #8743634 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/48757/#review45605 Good patch overall. We're almost there! ::: browser/components/newtab/NewTabMessages.jsm:150 (Diff revision 1) > + break; > + case ACTIONS.search.inSearch.performSearch: > + // Perform a search > + Task.spawn(function*() { > + try { > + yield NewTabSearchProvider.search.performSearch(target, data); You do not need to wrap this call in a Task nor use `yield` here. Calling the function will kick off the async task ::: browser/components/newtab/NewTabMessages.jsm:160 (Diff revision 1) > + break; > + case ACTIONS.search.inSearch.cycleEngine: > + // Set the new current engine > + Task.spawn(function*() { > + try { > + yield NewTabSearchProvider.search.cycleEngine(data); You do not need to wrap this call in a Task nor use `yield` here. Calling the function will kick off the async task ::: browser/components/newtab/NewTabSearchProvider.jsm:74 (Diff revision 1) > + } catch (e) { > + Cu.reportError(e); > + } > + }, > + > + get state() { This is an async function, as such, it should not be a property. Please rename to something like `asyncGetState` ::: browser/components/newtab/NewTabSearchProvider.jsm:89 (Diff revision 1) > + > + removeFormHistory({browser}, suggestion) { > + ContentSearch.removeFormHistoryEntry({target: browser}, suggestion); > + }, > + > + performSearch({browser}, searchData) { Please use Task.async. I also suggest a new convention: use async in the name of the function to ensure downstream users of the API are aware of the async nature ```javascript asyncPerformSearch: Task.async(function*({browser}, searchData) { ... }), ``` ::: browser/components/newtab/NewTabSearchProvider.jsm:101 (Diff revision 1) > + manageEngines(browser) { > + const browserWin = browser.ownerDocument.defaultView; > + browserWin.openPreferences("paneSearch"); > + }, > + > + cycleEngine(engineName) { please use Task.async, as above, with function name change ::: browser/components/newtab/NewTabSearchProvider.jsm:110 (Diff revision 1) > + let newEngine = state.currentEngine; > + this.emit(CURRENT_ENGINE, newEngine); > + }.bind(this)); > + }, > + > + getSuggestions(engineName, searchString, target) { please use Task.async, as above, with function name change ::: browser/components/newtab/tests/browser/browser_newtabsearchprovider.js:1 (Diff revision 1) > +"use strict"; Please make tests in this file an xpcshell test if possible. It appears we need mochitest or chrome, so we'll save considerable amounts of resources. One thing to watch out for is how ContentSearch behaves without a UI. You may need to do some manual initialization. ::: browser/modules/ContentSearch.jsm:129 (Diff revision 1) > } > this._searchSuggestionUIStrings = {}; > let searchBundle = Services.strings.createBundle("chrome://browser/locale/search.properties"); > let stringNames = ["searchHeader", "searchPlaceholder", "searchForSomethingWith", > "searchWithHeader", "searchSettings"]; > - for (let name of stringNames) { > + stringNames The chained version uses more resources: it allocates more memory, it: 1. creates a new array for each function call by map 2. creates an array to hold the map results 3. ultimately doesn't make use of the allocated memory and just sets an object's property What was wrong with the previous loop implementation? It looks clearer to me and is more efficient.
Attachment #8744982 - Attachment is obsolete: true
Attachment #8745108 - Attachment is obsolete: true
Attachment #8745114 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/49007/#review45845 A few promise/async things left ::: browser/components/newtab/NewTabMessages.jsm:148 (Diff revision 1) > + NewTabSearchProvider.search.removeFormHistory(target, suggestion); > + break; > + case ACTIONS.search.inSearch.performSearch: > + // Perform a search > + try { > + NewTabSearchProvider.search.asyncPerformSearch(target, data); You need to handle the error by calling `.catch` on the failed promise ::: browser/components/newtab/NewTabMessages.jsm:156 (Diff revision 1) > + } > + break; > + case ACTIONS.search.inSearch.cycleEngine: > + // Set the new current engine > + try { > + NewTabSearchProvider.search.asyncCycleEngine(data); You need to handle the error by calling `.catch` on the failed promise ::: browser/components/newtab/tests/xpcshell/test_NewTabSearchProvider.js:29 (Diff revision 1) > + > +add_task(function* test_state() { > + Services.search.init(function () { > + // get initial state of search and check it has correct properties > + do_check_true(Services.search.isInitialized); > + let state = yield NewTabSearchProvider.search.state; This is invalid, because the enclosing function is not a generator. Use `.then` ::: browser/components/newtab/tests/xpcshell/test_NewTabSearchProvider.js:68 (Diff revision 1) > + // set a new current engine > + Services.search.currentEngine = Services.search.getEngineByName(engineName); > + let expectedEngineName = Services.search.currentEngine.name; > + > + // emitter should fire and return the new engine > + let [eventName, actualEngineName] = yield promise; You need to use `.then` because the enclosing function is not a generator
https://reviewboard.mozilla.org/r/49007/#review45847 ::: browser/components/newtab/tests/xpcshell/test_NewTabSearchProvider.js:29 (Diff revision 1) > + > +add_task(function* test_state() { > + Services.search.init(function () { > + // get initial state of search and check it has correct properties > + do_check_true(Services.search.isInitialized); > + let state = yield NewTabSearchProvider.search.state; `NewTabSearchProvider.search.state` no longer exists. It should be `asyncGetState()`
Attachment #8745366 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/49055/#review45871 ::: browser/components/newtab/tests/xpcshell/test_NewTabSearchProvider.js:26 (Diff revision 1) > + ok(obj.hasOwnProperty(aProp), `expect to have property ${aProp}`); > + }; > +} > + > +add_task(function* test_state() { > + Services.search.init(function () { The tests will terminate before `search.init` finishes running. You might want to wrap it in a Promise, and yield the promise inside the generator. ::: browser/components/newtab/tests/xpcshell/test_NewTabSearchProvider.js:43 (Diff revision 1) > + }); > + }); > +}); > + > +add_task(function* test_observe() { > + Services.search.init(function() { as above
Attachment #8745624 - Attachment is obsolete: true
Attachment #8750347 - Attachment is obsolete: true
Attachment #8750801 - Flags: review?(oyiptong)
Comment on attachment 8750801 [details] MozReview Request: Bug 1210410 Implement search messages for the Remote New Tab https://reviewboard.mozilla.org/r/51641/#review49103
Attachment #8750801 - Flags: review?(oyiptong) → review+
https://hg.mozilla.org/mozilla-central/rev/b83c4529e13f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
https://reviewboard.mozilla.org/r/51641/#review48651 Looking good... mostly nits, but a few little things could be improved. As you already know/experienced, we've had some pretty terrible experience wrt promises not being `yield`'ed or .then()'ed .catch()'ed in the past, so I've tried to check for those things. Ursula, in the next round, let me know particular things you might want me to look for, etc. I'll try to avoid nit picking. And it's great to have you back full time!!!! :D :D :D ::: browser/components/newtab/NewTabMessages.jsm:90 (Diff revision 1) > > _prefs: {}, > > /** NEWTAB EVENT HANDLERS **/ > > handleContentRequest(actionName, {data, target}) { I'm still worried that `handleContentRequest` could potentially lead to some nasty promise-related bugs. The problem is that, in each case statement, Task.spawn() returns a promise that is dropped on the floor. I think the caller of `handleContentRequest()` should either receive the Promise resulting from each task (and just return Promise.resolve() for any sync task). so, like: ``` let task; switch(actionName){ case ACTIONS.search.inSearch.suggestions: // Return to the originator all search suggestions task = Task.spawn(function*() { try { let {engineName, searchString} = data; let suggestions = yield NewTabSearchProvider.search.asyncGetSuggestions(engineName, searchString, target); NewTabWebChannel.send(ACTIONS.search.outSearch.suggestions, suggestions, target); } catch (e) { Cu.reportError(e); } }); } // and at the end... return task; ``` Additionally, it would mean you wouldn't need try/catch or .catch() into Cu.reportError. You can make error reporting the responsibility of the calling code, which I think is better (e.g., you can gather all errors together and report them at once, etc.) - or the caller could so some error recovery. ::: browser/components/newtab/NewTabSearchProvider.jsm:32 (Diff revision 1) > +SearchProvider.prototype = { > + > + observe(subject, topic, data) { // jshint unused:false > + switch (data) { > + case "engine-current": > + if (topic === CURRENT_ENGINE) { nit: you can break early here too. if (topic !== CURRENT_ENGINE) { break; } ::: browser/components/newtab/NewTabSearchProvider.jsm:35 (Diff revision 1) > + switch (data) { > + case "engine-current": > + if (topic === CURRENT_ENGINE) { > + Task.spawn(function* () { > + try { > + let state = yield ContentSearch.currentStateObj(true); See other comment about boolean trap. If this can't be fixed, just add: ```js /*uriFlag=*/true ``` So at least it's clear ::: browser/components/newtab/NewTabSearchProvider.jsm:58 (Diff revision 1) > + > + init() { > + try { > + Services.obs.addObserver(this, CURRENT_ENGINE, true); > + } catch (e) { > + Cu.reportError(e); Maybe rethrow the error. Is there much point in continuing if the init() failed? ::: browser/components/newtab/NewTabSearchProvider.jsm:92 (Diff revision 1) > + asyncGetState: Task.async(function*() { > + let state = yield ContentSearch.currentStateObj(true); > + return state; > + }), > + > + asyncPerformSearch: Task.async(function*({browser}, searchData) { Nit: You could destructure browser into target, so: ``` asyncPerformSearch: Task.async(function*({browser: target}, searchData) { ContentSearch.performSearch({target}, searchData); yield ContentSearch.addFormHistoryEntry({target}, searchData.searchString); } ``` Same in `removeFormHistory`. Could make things a little bit neater ::: browser/components/newtab/tests/browser/browser_newtabmessages.js:187 (Diff revision 1) > + url: testURL > + }; > + > + let UIStringsResponseAck = new Promise(resolve => { > + NewTabWebChannel.once("UIStringsAck", (_, msg) => { > + ok(true, "a search request response for UI string has been received"); Nit: this assertion is more of a debug statement. I don't think you should include this. ::: browser/components/newtab/tests/browser/browser_newtabmessages.js:215 (Diff revision 1) > + resolve(); > + }); > + }); > + > + yield BrowserTestUtils.withNewTab(tabOptions, function*() { > + yield UIStringsResponseAck; The yields here implies ordering matters, but it actually doesn't (as all the promises could settle at different times). Thus, it's probaby better to use: ```js Promise.all([ UIStringsResponseAck, suggestionsResponseAck, stateResponseAck, currentEngineResponseAck, ]) ``` In fact, you could also just collapse all the tests above too: ```js function awaitMessage(messageName, testString){ return new Promise(resolve => { NewTabWebChannel.once(messageName, (_, msg) => { ok(true, `a search request response for ${messageName} has been received`); ok(msg.data, testString); resolve(); }); }); } ///... in add_task const ackPromises = [ awaitMessage("UIStringsAck", "received the UI Strings"), awaitMessage("suggestionsAck", "received the suggestions"), awaitMessage("stateAck", "received a state object"), awaitMessage("currentEngineAck", "received a current engine"), ] yield BrowserTestUtils.withNewTab(tabOptions, function*() { yield Promise.all(ackPromises); }) yield cleanup(); ``` ::: browser/components/newtab/tests/browser/browser_newtabmessages.js:221 (Diff revision 1) > + yield suggestionsResponseAck; > + yield stateResponseAck; > + yield currentEngineResponseAck; > + }); > + > + cleanup(); q: no `yield cleanup()`? ::: browser/components/newtab/tests/browser/newtabmessages_search.html:22 (Diff revision 1) > + searchString: "test", > + }; > + let cycleEngineData = "Engine2"; > + > + window.addEventListener("WebChannelMessageToContent", function(e) { > + if (e.detail.message) { Nit: return early so to avoid needlessly nested code. I.e., ```JS if(!e.detail.message){ return; } ``` ::: browser/components/newtab/tests/browser/newtabmessages_search.html:23 (Diff revision 1) > + }; > + let cycleEngineData = "Engine2"; > + > + window.addEventListener("WebChannelMessageToContent", function(e) { > + if (e.detail.message) { > + let reply; nit: declare reply in place where you are going to use it (i.e., inside each case, see my other comment). By declaring it at the top, it implies you are going to use it outside the switch statement. ::: browser/components/newtab/tests/browser/newtabmessages_search.html:26 (Diff revision 1) > + window.addEventListener("WebChannelMessageToContent", function(e) { > + if (e.detail.message) { > + let reply; > + switch (e.detail.message.type) { > + case "RECEIVE_UISTRINGS": > + reply = new window.CustomEvent("WebChannelMessageToChrome", { Nit, make message a variable, because if things go bad, it makes it easier to debug. I would do: ```js const message = JSON.stringify({type: "UIStringsAck", data: e.detail.message.data}; const detail = { id: "newtab", message, }; const reply = new window.CustomEvent("WebChannelMessageToChrome", {detail} window.dispatchEvent(reply); ``` ::: browser/components/newtab/tests/browser/newtabmessages_search.html:63 (Diff revision 1) > + }); > + window.dispatchEvent(reply); > + break; > + } > + } > + }, true); Nit: why true? i.e., you probably don't care about event phase, so just remove the last arg. ::: browser/components/newtab/tests/browser/newtabmessages_search.html:66 (Diff revision 1) > + } > + } > + }, true); > + > + document.onreadystatechange = function () { > + if (document.readyState === "complete") { as above, return early: ```JS if (document.readyState !== "complete") { return; } ``` Also, any particular reason why you are not just using window.onload or "DOMContentLoaded"? readystatechange fires multiple times, which you don't use - so maybe just `onload` would be sufficient (which would mean the document.readyState would be complete, and you could also drop that line) ::: browser/components/newtab/tests/browser/newtabmessages_search.html:108 (Diff revision 1) > + detail: { > + id: "newtab", > + message: JSON.stringify({type: "REQUEST_CYCLE_ENGINE", data: cycleEngineData}), > + } > + }); > + window.dispatchEvent(msg); You are going to hate me, but the seperation between sending the messages and recieving the messages is not great, IMO. It means that messages can come back in unpredicatable order making it hard to debug. Ideally, maybe would want something like (no idea if this works, I didn't run it or anything): ```JS "use strict"; const tests = new Map([ // <String type, Object? {data}> ["REQUEST_UISTRINGS", null], ["REQUEST_SEARCH_SUGGESTIONS", { data: suggestionsData }], ["REQUEST_SEARCH_STATE", null], ["REQUEST_REMOVE_FORM_HISTORY", { data: removeFormHistoryData }], ["REQUEST_PERFORM_SEARCH", { data: performSearchData }], ["REQUEST_CYCLE_ENGINE", { data: cycleEngineData }], ]); /** * Send message up to the Chrome * @param String type **/ function makeOutboundEvent(type) { const message = JSON.stringify({type}); const detail = { id: "newtab", message, }; const event = new window.CustomEvent("WebChannelMessageToChrome", detail); return event; } /** * Awaits an event response, * @param Event * @returns Promise settles when we get a response **/ function awaitResponse({ type }) { return new Promise((resolve, reject) => { window.addEventListener("WebChannelMessageToContent", function handler(e) { if (e.type !== type) { console.warn(`Got event, but it was not my type? ${e.type}`); return; } window.removeEventListener("WebChannelMessageToContent", handler); if (!e.detail.message) { return reject(new Error("No message in the detail!")); } const { data } = e.detail.message; // Just tack _ACK onto the end of the type const message = JSON.stringify({ type: type + "_ACK", data }); const detail = { id: "newtab", message, }; const reply = new window.CustomEvent("WebChannelMessageToChrome", { detail }); window.dispatchEvent(reply); resolve(); }); }); } window.onload = function() { async.task(function* runTests() { for (const [type, data] of tests.entries()) { const event = makeOutboundEvent(type); window.dispatchEvent(event); try { yield awaitResponse(event); } catch (err) { console.error(type, data, err); } } }) // Catch any unforseen errors .catch(err => console.error(err)); }; ``` ::: browser/modules/ContentSearch.jsm:334 (Diff revision 1) > - }, > + }, > - }); > + }); > + return true; > + }), > + > + currentStateObj: Task.async(function* (uriFlag=false) { Boolean trap! "It's a trap!" :) http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html Could we refactor this to be an object: ```js currentStateObj: Task.async(function* (options = {uriFlag=false}) { ```
Depends on: 1277668
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: