Closed
Bug 1041429
Opened 10 years ago
Closed 10 years ago
Adopt template strings in UnifiedComplete
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: asaf, Assigned: michael, Mentored)
References
Details
(Whiteboard: [good first bug][lang=js])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
Now that template strings are implemented, all that string.replace mess in UnifiedComplete may be cleaned up nicely.
Reporter | ||
Comment 1•10 years ago
|
||
also in _processRow and probably in few other places
Comment 2•10 years ago
|
||
Yeah, let's do this. Can you mentor this? It seems like a good first or second bug, no?
Flags: needinfo?(mano)
OS: Mac OS X → All
Hardware: x86 → All
Comment 4•10 years ago
|
||
We actually have a separate bugzilla field for that now :)
Mentor: mano
Whiteboard: [mentor=mano] → [good-first-bug][lang=js]
Reporter | ||
Comment 5•10 years ago
|
||
Nice.
***
function sql(...parts) parts.join(" ") can also be removed in favor of "no substitution template" string usage.
Updated•10 years ago
|
Blocks: UnifiedComplete
Updated•10 years ago
|
Whiteboard: [good-first-bug][lang=js] → [good first bug][lang=js]
Assignee | ||
Comment 6•10 years ago
|
||
I've updated UnifiedComplete.js to use template strings in place of String.replace() where appropriate. I've also replaced uses of the sql() function with multi-line strings.
https://tbpl.mozilla.org/?tree=Try&rev=b65757c0edc3
Attachment #8460881 -
Flags: review?(mano)
Comment 7•10 years ago
|
||
Template strings are currently #ifdef NIGHTLY_BUILD, it's probably a good idea to put any consumers behind that as well to avoid surprises at Aurora uplift time.
Bug 1038259 removes the flag. It's not landed yet, but won't be too long.
Reporter | ||
Comment 9•10 years ago
|
||
Comment on attachment 8460881 [details] [diff] [review]
Patch
Review of attachment 8460881 [details] [diff] [review]:
-----------------------------------------------------------------
Unfortunately the patch doesn't apply anymore (my fault for not reviewing it promptly). Given the nature of this patch (most of the changes are in a single, quite large hunk), you may want to back out the "breaking" patches locally, reapply your patch, reapply the other patches, find out if there are more places which need update (any new sql/replace usage), and then generate the new patch. Please re-run the tests. Autocomplete code tends to break easily.
Sorry for that :(
::: toolkit/components/places/UnifiedComplete.js
@@ +100,5 @@
> // TODO bug 412736: in case of a frecency tie, we might break it with h.typed
> // and h.visit_count. That is slower though, so not doing it yet...
> +function defaultQuery(conditions) {
> + if (conditions === undefined)
> + conditions = "";
Use the default argument syntax.
function defaultQuery(conditions = "") {
@@ +189,4 @@
>
> +function hostQuery(conditions) {
> + if (conditions === undefined)
> + conditions = "";
Ditto.
@@ +207,5 @@
> +const SQL_TYPED_HOST_QUERY = hostQuery("AND typed = 1");
> +
> +function urlQuery(conditions) {
> + if (conditions === undefined)
> + conditions = "";
Same Same. Please double check if I overlooked any instance of that.
Attachment #8460881 -
Flags: review?(mano) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Thanks, I've updated the patch to the most recent version of UnifiedComplete.js. I've updated the query functions to use the default argument syntax as you suggested.
Here are the results from the try server:
https://tbpl.mozilla.org/?tree=Try&rev=664510e6aa77
Assignee: nobody → michael
Attachment #8460881 -
Attachment is obsolete: true
Attachment #8468531 -
Flags: review?(mano)
Reporter | ||
Updated•10 years ago
|
Attachment #8468531 -
Flags: review?(mano) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 11•10 years ago
|
||
Keywords: checkin-needed
Comment 12•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
QA Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•