Update UseCounter::deprecationMessage() cases to use the replacedBy function

Update UseCounter::deprecationMessage() cases to use the replacedBy function

The updated messages can use the replacedBy() function to generate the message.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188205

[kkristof, 2014-11-10 17:26:53]

kkosztyo.u-szeged@partner.samsung.com changed reviewers:
+ darin@chromium.org, jchaffraix@chromium.org, vsevik@chromium.org

[Julien - ping for review, 2014-11-18 00:24:33]

We don't put an empty BUG= line in the description.

Also your change should have a description as to *why* this change is good.
Without context, it makes it hard to understand what you're trying to do (and
help you get there if you made some mistakes in the direction).

https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.cpp:667: // FIXME: Update other
UseCounter::deprecationMessage() cases to use this.
Shouldn't this be removed?

https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.cpp:668: static String replacedBy(const char*
oldString, const char* newString, bool useApostrophe = true)
Booleans are not a good idea in a function's signature (good reading on this:
http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html). They make
the callers difficult to read, especially with a default parameter.

https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.cpp:681: return
replacedBy("window.webkitStorageInfo", "navigator.webkitTemporaryStorage' or
'navigator.webkitPersistentStorage");
This call is just magical. If you don't know what the function is doing it's
impossible to understand why you need the single quotes.

If you look at the callers, they have to *know* how the function is implemented
below the API. That means that you're function's implementation is leaking to
the outside world, which is a bad smell.

https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.cpp:703: return
replacedBy("'HTMLVideoElement.webkitDisplayingFullscreen'", "the
'fullscreenchange' and 'webkitfullscreenchange' events", false);
I think your mistake here is to assume that we *have* to keep the different
messages as they are. If you remove this artificial constraint and decide to
change some of them for the sake of consistency (which is great to have!), you
can make a better API decision:

void replaceBy(const char* deprecatedItem, const char** replacementItems)
{
    size_t replacementItemsSize = sizeof(replacementItems) /
sizeof(replacementItems[0]); // That's assuming |replacementItems| is on the
stack, which should be the case.
    // ...
}

https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.cpp:799: return replacedBy("The 'vspace' attribute
on table", "CSS", false);
This should be moved to a more useful suggestion: margin-top and margin-bottom

https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.cpp:802: return replacedBy("The 'hspace' attribute
on table", "CSS", false);
Same comment here: margin-left and margin-right

[kkristof, 2014-11-18 08:15:34]

On 2014/11/18 00:24:33, Julien Chaffraix - PST wrote:
> We don't put an empty BUG= line in the description.

Yes, I missed this.

> 
> Also your change should have a description as to *why* this change is good.
> Without context, it makes it hard to understand what you're trying to do (and
> help you get there if you made some mistakes in the direction).
> 
>
https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
> File Source/core/frame/UseCounter.cpp (right):
> 
>
https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
> Source/core/frame/UseCounter.cpp:667: // FIXME: Update other
> UseCounter::deprecationMessage() cases to use this.
> Shouldn't this be removed?
> 
>
https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
> Source/core/frame/UseCounter.cpp:668: static String replacedBy(const char*
> oldString, const char* newString, bool useApostrophe = true)
> Booleans are not a good idea in a function's signature (good reading on this:
> http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html). They
make
> the callers difficult to read, especially with a default parameter.
> 
>
https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
> Source/core/frame/UseCounter.cpp:681: return
> replacedBy("window.webkitStorageInfo", "navigator.webkitTemporaryStorage' or
> 'navigator.webkitPersistentStorage");
> This call is just magical. If you don't know what the function is doing it's
> impossible to understand why you need the single quotes.
> 
> If you look at the callers, they have to *know* how the function is
implemented
> below the API. That means that you're function's implementation is leaking to
> the outside world, which is a bad smell.
> 
>
https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
> Source/core/frame/UseCounter.cpp:703: return
> replacedBy("'HTMLVideoElement.webkitDisplayingFullscreen'", "the
> 'fullscreenchange' and 'webkitfullscreenchange' events", false);
> I think your mistake here is to assume that we *have* to keep the different
> messages as they are. If you remove this artificial constraint and decide to
> change some of them for the sake of consistency (which is great to have!), you
> can make a better API decision:
> 
> void replaceBy(const char* deprecatedItem, const char** replacementItems)
> {
>     size_t replacementItemsSize = sizeof(replacementItems) /
> sizeof(replacementItems[0]); // That's assuming |replacementItems| is on the
> stack, which should be the case.
>     // ...
> }

Yes you are right. It will needs to update several test expectations what I
tried to avoid.

> 
>
https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
> Source/core/frame/UseCounter.cpp:799: return replacedBy("The 'vspace'
attribute
> on table", "CSS", false);
> This should be moved to a more useful suggestion: margin-top and margin-bottom
> 
>
https://codereview.chromium.org/717493003/diff/1/Source/core/frame/UseCounter...
> Source/core/frame/UseCounter.cpp:802: return replacedBy("The 'hspace'
attribute
> on table", "CSS", false);
> Same comment here: margin-left and margin-right

If the expectations are updated then these can change easily.

Thank you for the review :)

[vsevik, 2014-11-20 09:19:43]

vsevik@chromium.org changed reviewers:
- vsevik@chromium.org

[kkristof, 2014-12-10 16:21:28]

Hi, I have updated the patch. Please take a look.

[Julien - ping for review, 2014-12-12 00:33:28]

jchaffraix@chromium.org changed reviewers:
+ thakis@chromium.org

[Julien - ping for review, 2014-12-12 00:33:29]

I don't know enough C++11 to actually review this change. The idea of the change
is fine but CC'ing Nico for the C++11 magic.

https://codereview.chromium.org/717493003/diff/20001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/717493003/diff/20001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:668: template <typename... T>
Variadic template is allowed but seems to be troubled territory
(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/6ItymeMXpMc).

My understanding is that this would also allow for 0 |replacementItems| which is
not great.

[Nico, 2014-12-12 03:57:49]

https://codereview.chromium.org/717493003/diff/20001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/717493003/diff/20001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:674: for (const char* replacementItem : {
replacementItems... }) {
I think iterating over an initializer list requires `#include
<initializer_list>`, which is a c++11 library thing, and c++11 library things
don't work on all platforms yet. So I'd expect that this won't compile on all
platforms.

[Julien - ping for review, 2014-12-12 17:27:46]

https://codereview.chromium.org/717493003/diff/20001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/717493003/diff/20001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:671: String replacementMessage;
Probably better to use a StringBuilder even if the number of strings is small.

https://codereview.chromium.org/717493003/diff/20001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:674: for (const char* replacementItem : {
replacementItems... }) {
On 2014/12/12 03:57:49, Nico wrote:
> I think iterating over an initializer list requires `#include
> <initializer_list>`, which is a c++11 library thing, and c++11 library things
> don't work on all platforms yet. So I'd expect that this won't compile on all
> platforms.

Thanks Nico, it seems like we should just stick with a simple char* array then.

[kkristof, 2014-12-16 15:27:30]

Hi,
I have updated the patch. I removed the quoting from the replacedBy function so
the replace message can be more descriptive in some cases but it needs test
expectations update.

[Julien - ping for review, 2014-12-16 16:32:40]

jchaffraix@chromium.org changed reviewers:
+ jsbell@chromium.org

[Julien - ping for review, 2014-12-16 16:32:41]

+Joshua as the authors of the FIXME and deprecator extraordinaire, in case he
has some comment.

lgtm

https://codereview.chromium.org/717493003/diff/40001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/717493003/diff/40001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:674: return String::format("%s is deprecated.
Please use %s instead.", oldString, newString);
You're removing the quoting, which was neat, but I won't fight for it.

[jsbell, 2014-12-16 16:39:38]

On 2014/12/16 16:32:41, Julien Chaffraix - PST wrote:
> +Joshua as the authors of the FIXME and deprecator extraordinaire, in case he
> has some comment.
> 
> lgtm
> 
>
https://codereview.chromium.org/717493003/diff/40001/Source/core/frame/UseCou...
> File Source/core/frame/UseCounter.cpp (right):
> 
>
https://codereview.chromium.org/717493003/diff/40001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.cpp:674: return String::format("%s is deprecated.
> Please use %s instead.", oldString, newString);
> You're removing the quoting, which was neat, but I won't fight for it.

Let's keep the quotes.

The other uses weren't updated just to reduce test result churn. This will
likely require a lot of expected result updates, since console spew is recorded.
I'll kick off a try job once I'm in the office.

[kkristof, 2014-12-17 14:56:22]

> Let's keep the quotes.
> 
> The other uses weren't updated just to reduce test result churn. This will
> likely require a lot of expected result updates, since console spew is
recorded.
> I'll kick off a try job once I'm in the office.

I have updated, now it keeps the quoting so in some cases we can't use the
replacedBy function.

[jsbell, 2014-12-17 21:29:19]

I ran another try job, relevant layout test results here:

https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...

The non-timeouts look as you'd expect, so can you run those locally with
--reset-results to update new baselines, and include them in the patch?

I'm not sure if the timeouts are related or a flake.

[jsbell, 2014-12-18 20:05:50]

I'll kick off another try job to see how the changes look, but now that I'm
actually looking at the output...

https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
File Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:672: static String replacedBy(const char*
deprecatedItem, const char* firstAlternative, const char* secondAlternative =
nullptr)
This is fine, but IMHO it would be more readable to have two methods rather the
default value for the second arg, since the if/else block and replacementMessage
variable can go away.

(I realize the patch used to be more complex made this function variadic.)

https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:701: return replacedBy("showModalDialog",
"window.open", "postMessage");
The new message is a bit misleading. You don't use window.open *OR* postMessage,
you'd use them both together.

So I'd leave this one alone, or perhaps ask on chromium-dev if anyone has a
better message here. (e.g. we could actually include the URL of a document like
https://dev.opera.com/blog/showmodaldialog/ that gives advice)

https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:710: return
replacedBy("HTMLVideoElement.webkitDisplayingFullscreen", "fullscreenchange",
"webkitfullscreenchange");
Can we skip recommending webkitfullscreenchange and only recommend the
standardized version?

Also, this change drops the key word 'events'. Not sure how critical that
context is.

https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:713: return
replacedBy("HTMLVideoElement.webkitEnterFullscreen()",
"Element.requestFullscreen()", "Element.webkitRequestFullscreen()");
Can we skip recommending the nonstandard prefixed version and only recommend the
standardized version?

https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:716: return
replacedBy("HTMLVideoElement.webkitExitFullscreen()",
"Document.exitFullscreen()", "Document.webkitExitFullscreen()");
Can we skip recommending the nonstandard prefixed version and only recommend the
standardized version?

https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:719: return
replacedBy("HTMLVideoElement.webkitEnterFullScreen()",
"Element.requestFullscreen()", "Element.webkitRequestFullscreen()");
Can we skip recommending the nonstandard prefixed version and only recommend the
standardized version?

https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
Source/core/frame/UseCounter.cpp:722: return
replacedBy("HTMLVideoElement.webkitExitFullScreen()",
"Document.exitFullscreen()", "Document.webkitExitFullscreen()");
Can we skip recommending the nonstandard prefixed version and only recommend the
standardized version?

[kkristof, 2014-12-19 15:28:49]

On 2014/12/18 20:05:50, jsbell wrote:
> I'll kick off another try job to see how the changes look, but now that I'm
> actually looking at the output...
> 
>
https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
> File Source/core/frame/UseCounter.cpp (right):
> 
>
https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.cpp:672: static String replacedBy(const char*
> deprecatedItem, const char* firstAlternative, const char* secondAlternative =
> nullptr)
> This is fine, but IMHO it would be more readable to have two methods rather
the
> default value for the second arg, since the if/else block and
replacementMessage
> variable can go away.
> 
> (I realize the patch used to be more complex made this function variadic.)
> 
>
https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.cpp:701: return replacedBy("showModalDialog",
> "window.open", "postMessage");
> The new message is a bit misleading. You don't use window.open *OR*
postMessage,
> you'd use them both together.
> 
> So I'd leave this one alone, or perhaps ask on chromium-dev if anyone has a
> better message here. (e.g. we could actually include the URL of a document
like
> https://dev.opera.com/blog/showmodaldialog/ that gives advice)
> 
>
https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.cpp:710: return
> replacedBy("HTMLVideoElement.webkitDisplayingFullscreen", "fullscreenchange",
> "webkitfullscreenchange");
> Can we skip recommending webkitfullscreenchange and only recommend the
> standardized version?
> 
> Also, this change drops the key word 'events'. Not sure how critical that
> context is.
> 
>
https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.cpp:713: return
> replacedBy("HTMLVideoElement.webkitEnterFullscreen()",
> "Element.requestFullscreen()", "Element.webkitRequestFullscreen()");
> Can we skip recommending the nonstandard prefixed version and only recommend
the
> standardized version?
> 
>
https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.cpp:716: return
> replacedBy("HTMLVideoElement.webkitExitFullscreen()",
> "Document.exitFullscreen()", "Document.webkitExitFullscreen()");
> Can we skip recommending the nonstandard prefixed version and only recommend
the
> standardized version?
> 
>
https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.cpp:719: return
> replacedBy("HTMLVideoElement.webkitEnterFullScreen()",
> "Element.requestFullscreen()", "Element.webkitRequestFullscreen()");
> Can we skip recommending the nonstandard prefixed version and only recommend
the
> standardized version?
> 
>
https://codereview.chromium.org/717493003/diff/80001/Source/core/frame/UseCou...
> Source/core/frame/UseCounter.cpp:722: return
> replacedBy("HTMLVideoElement.webkitExitFullScreen()",
> "Document.exitFullscreen()", "Document.webkitExitFullscreen()");
> Can we skip recommending the nonstandard prefixed version and only recommend
the
> standardized version?

Hi, I have updated the patch.
I've removed the modifications of the replacedBy function because it would be
used only one case after the webkit prefixed replacements were deleted from the
other cases.

[jsbell, 2014-12-19 23:36:02]

Well that's simpler. :)

lgtm but please inspect any layout test failures (don't assume they're flakes)

[kkristof, 2015-01-09 16:05:32]

The CQ bit was checked by kkosztyo.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2015-01-09 16:05:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/717493003/120001

[commit-bot: I haz the power, 2015-01-09 17:47:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-09 17:47:05]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/45527)

[kkristof, 2015-01-12 07:43:31]

The CQ bit was checked by kkosztyo.u-szeged@partner.samsung.com

[commit-bot: I haz the power, 2015-01-12 07:45:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/717493003/140001

[commit-bot: I haz the power, 2015-01-12 08:57:29]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188205