Closed Bug 1057901 Opened 5 years ago Closed 5 years ago

Use template strings for multi-line SQL statements in content preferences

Categories

(Toolkit :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: michael, Assigned: michael)

Details

Attachments

(1 file, 2 obsolete files)

Following the changes in bug 1041429 adopting template strings in UnifiedComplete.js, I propose using template strings for multi-line SQL statements in content preferences code.
Attached patch Patch (obsolete) — Splinter Review
Here are the results from the try server:

https://tbpl.mozilla.org/?tree=Try&rev=4048eaa5319d
Attachment #8478040 - Flags: review?(adw)
Comment on attachment 8478040 [details] [diff] [review]
Patch

Review of attachment 8478040 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, thanks Michael.  It's too bad this'll make following blame a little harder for not a lot of benefit.  I'm not sure it's even worth it.

I'd prefer this style:

let s = foo(`
  SELECT huh
  FROM yeah
`);

But this is a pretty big patch, so I won't ask you to change everything unless you want to.
Attachment #8478040 - Flags: review?(adw) → review+
Comment on attachment 8478040 [details] [diff] [review]
Patch

Review of attachment 8478040 [details] [diff] [review]:
-----------------------------------------------------------------

Oh wait, this needs to remove joinArgs from ContentPrefService2.jsm and make sure all callers are updated to no longer need it.  No caller should need it anymore now that we're using template strings, but I think you'll have to adjust some.
Attachment #8478040 - Flags: review+
Attached patch Patch (obsolete) — Splinter Review
I have updated the style of the template strings and have removed joinArgs() as you suggested. I've also eliminated the explicit call to String.prototype.replace() when creating a statement with a group clause.

Here are the results from the try server:

https://tbpl.mozilla.org/?tree=Try&rev=cfc6938ccfc6
Attachment #8478040 - Attachment is obsolete: true
Attachment #8478806 - Flags: review?(adw)
Comment on attachment 8478806 [details] [diff] [review]
Patch

Review of attachment 8478806 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Michael.

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +32,4 @@
>  Cu.import("resource://gre/modules/ContentPrefUtils.jsm");
>  Cu.import("resource://gre/modules/ContentPrefStore.jsm");
>  
> +const kGroupClause = `

Please call this GROUP_CLAUSE to match current naming conventions for consts.
Attachment #8478806 - Flags: review?(adw) → review+
Attached patch PatchSplinter Review
I have changed kGroupClause to GROUP_CLAUSE as you suggested.
Attachment #8478806 - Attachment is obsolete: true
Attachment #8479451 - Flags: review?(adw)
Comment on attachment 8479451 [details] [diff] [review]
Patch

Review of attachment 8479451 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Michael.  For future reference, when your patch is review+'ed but your reviewer asks you to change some things anyway, you don't need to request review again on the new patch -- unless of course you feel that it's warranted by your changes.  But I'm happy to r+ this again, so it's no problem at all. :-)
Attachment #8479451 - Flags: review?(adw) → review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/2864580f60b8

Thanks for the patch, Michael!
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2864580f60b8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.