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

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Michael Pruett]

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.

Comment 1

[Michael Pruett]

Attachment: Patch

Here are the results from the try server:

https://tbpl.mozilla.org/?tree=Try&rev=4048eaa5319d

Comment 2

[Drew Willcoxon :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.

Comment 3

[Drew Willcoxon :adw]

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.

Comment 4

[Michael Pruett]

Attachment: Patch

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

Comment 5

[Drew Willcoxon :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.

Comment 6

[Michael Pruett]

Attachment: Patch

I have changed kGroupClause to GROUP_CLAUSE as you suggested.

Comment 7

[Drew Willcoxon :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. :-)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2864580f60b8

Thanks for the patch, Michael!

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2864580f60b8