Closed
Bug 1487125
Opened 6 years ago
Closed 6 years ago
Write basic Query Context and Address Bar Controller classes
Categories
(Firefox :: Address Bar, enhancement, P1)
Firefox
Address Bar
Tracking
()
RESOLVED
FIXED
Firefox 64
People
(Reporter: standard8, Assigned: standard8)
References
()
Details
(Whiteboard: [fxsearch])
User Story
- Create outlines of the new classes. - Make the AddressBarController "work" to the extent that it can handle passing basic searches from the view to the model and return results (with stubbing of the model and view where necessary).
Attachments
(2 files)
As an early part of the address bar rewrite, we want to create some basic forms of QueryContext and AddressBarController, that we can add to as we develop the new address bar architecture.
The main aims of this bug are outlined in the user story.
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
This is based around using promises for query completion.
Depends on D4566
Assignee | ||
Comment 3•6 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #2)
> Created attachment 9006363 [details]
> Bug 1487125 - Create a basic UrlbarController object for the address bar
> rewrite.
This is a WIP patch that is slightly different to the original design for the controller. It uses promises for the view side of the interface rather than a listener based system - Mike suggested this a couple of times, so I thought I'd play around with it.
I think it might be OK, as long as we are happy that the controller will only ever pass the view a complete set of results, and there will be no option for results being passed in blocks.
I believe that was a limitation we previously decided on, due to issues with updating the UI multiple times, but I can't find the docs for it.
We could also use a promise on the model side of the controller as well, Marco what do you think about that?
If you folks could take a look at the patch(es), and see what you think to the above limitation, and using promises, that would be very useful before I finalise this for review.
Flags: needinfo?(mak77)
Flags: needinfo?(dao+bmo)
Flags: needinfo?(adw)
Assignee | ||
Comment 4•6 years ago
|
||
I discussed with Marco on irc. He pointed out that we need to have the potential for multiple result sets to the UI per query, as this gives the minimum latency possible.
Therefore the promise based approach isn't really sensible here. If we change how things work in the future, then we could revert to it, but I think it is simpler for now to have listener based approach.
Flags: needinfo?(mak77)
Flags: needinfo?(dao+bmo)
Flags: needinfo?(adw)
Comment 5•6 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #3)
> I think it might be OK, as long as we are happy that the controller will
> only ever pass the view a complete set of results, and there will be no
> option for results being passed in blocks.
>
> I believe that was a limitation we previously decided on, due to issues with
> updating the UI multiple times, but I can't find the docs for it.
That's not how I remember it. I thought incremental updates were a crucial requirement. I don't have all the context for why this would be crucial, though... I think basically some results can take longer to retrieve than others, and we want to give something to the user ASAP? Not doing incremental updates makes my life a bit easier, so if mak thinks this okay I'm not going to object.
Comment 6•6 years ago
|
||
Mark, I saw your comment in mid-air, submitted mine anyway. Seems like we're on the same page.
Assignee | ||
Comment 7•6 years ago
|
||
I think the patches are in a reasonable place now that we can start reviews. I think there's going to be lots of mutations as we get going, but hopefully this is a good base.
Assignee | ||
Updated•6 years ago
|
Status: NEW → ASSIGNED
Comment 8•6 years ago
|
||
Just to chime in, I agree with the other comments that multiple result sets and incremental updates are important.
Comment 9•6 years ago
|
||
Comment on attachment 9004928 [details]
Bug 1487125 - Create a basic QueryContext object for the Address Bar architecture rewrite.
Marco Bonardo [::mak] has approved the revision.
Attachment #9004928 -
Flags: review+
Comment 10•6 years ago
|
||
Comment on attachment 9006363 [details]
Bug 1487125 - Create a basic UrlbarController object for the address bar rewrite.
Marco Bonardo [::mak] has approved the revision.
Attachment #9006363 -
Flags: review+
Comment 11•6 years ago
|
||
Comment on attachment 9006363 [details]
Bug 1487125 - Create a basic UrlbarController object for the address bar rewrite.
Dão Gottwald [::dao] has approved the revision.
Attachment #9006363 -
Flags: review+
Comment 12•6 years ago
|
||
Comment on attachment 9004928 [details]
Bug 1487125 - Create a basic QueryContext object for the Address Bar architecture rewrite.
Dão Gottwald [::dao] has approved the revision.
Attachment #9004928 -
Flags: review+
Comment 13•6 years ago
|
||
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa6fb040f5b6
Create a basic QueryContext object for the Address Bar architecture rewrite. r=mak,dao
Comment 14•6 years ago
|
||
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96254477baa8
Create a basic UrlbarController object for the address bar rewrite. r=mak,dao
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fa6fb040f5b6
https://hg.mozilla.org/mozilla-central/rev/96254477baa8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•