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

Source/core/frame/UseCounter.cpp

 
 /*
  * Copyright (C) 2012 Google, Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 646 matching lines @@
     UseCounter::countDeprecation(document.frame(), feature);
 }
 
 void UseCounter::countDeprecationIfNotPrivateScript(v8::Isolate* isolate, ExecutionContext* context, Feature feature)
 {
     if (DOMWrapperWorld::current(isolate).isPrivateScriptIsolatedWorld())
         return;
     UseCounter::countDeprecation(context, feature);
 }
 
 // FIXME: Update other UseCounter::deprecationMessage() cases to use this.

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

Shouldn't this be removed?

-static String replacedBy(const char* oldString, const char* newString)
+static String replacedBy(const char* oldString, const char* newString, bool useApostrophe = true)

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

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.

 {
-    return String::format("'%s' is deprecated. Please use '%s' instead.", oldString, newString);
+    if (useApostrophe)
+        return String::format("'%s' is deprecated. Please use '%s' instead.", oldString, newString);
+
+    return String::format("%s is deprecated. Please use %s instead.", oldString, newString);
 }
 
 String UseCounter::deprecationMessage(Feature feature)
 {
     switch (feature) {
     // Quota
     case PrefixedStorageInfo:
-        return "'window.webkitStorageInfo' is deprecated. Please use 'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage' instead.";
+        return replacedBy("window.webkitStorageInfo", "navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage");

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

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.

 
     // Keyboard Event (DOM Level 3)
     case KeyboardEventKeyLocation:
         return replacedBy("KeyboardEvent.keyLocation", "KeyboardEvent.location");
 
     case ConsoleMarkTimeline:
-        return "console.markTimeline is deprecated. Please use the console.timeStamp instead.";
+        return replacedBy("console.markTimeline", "the console.timeStamp", false);
 
     case FileError:
         return "FileError is deprecated. Please use the 'name' or 'message' attributes of DOMError rather than 'code'.";
 
     case ShowModalDialog:
-        return "showModalDialog is deprecated. Please use window.open and postMessage instead.";
+        return replacedBy("showModalDialog", "window.open and postMessage");
 
     case CSSStyleSheetInsertRuleOptionalArg:
         return "Calling CSSStyleSheet.insertRule() with one argument is deprecated. Please pass the index argument as well: insertRule(x, 0).";
 
     case PrefixedVideoSupportsFullscreen:
         return "'HTMLVideoElement.webkitSupportsFullscreen' is deprecated. Its value is true if the video is loaded.";
 
     case PrefixedVideoDisplayingFullscreen:
-        return "'HTMLVideoElement.webkitDisplayingFullscreen' is deprecated. Please use the 'fullscreenchange' and 'webkitfullscreenchange' events instead.";
+        return replacedBy("'HTMLVideoElement.webkitDisplayingFullscreen'", "the 'fullscreenchange' and 'webkitfullscreenchange' events", false);

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

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.
    // ...
}

 
     case PrefixedVideoEnterFullscreen:
-        return "'HTMLVideoElement.webkitEnterFullscreen()' is deprecated. Please use 'Element.requestFullscreen()' and 'Element.webkitRequestFullscreen()' instead.";
+        return replacedBy("HTMLVideoElement.webkitEnterFullscreen()", "Element.requestFullscreen()' and 'Element.webkitRequestFullscreen()");
 
     case PrefixedVideoExitFullscreen:
-        return "'HTMLVideoElement.webkitExitFullscreen()' is deprecated. Please use 'Document.exitFullscreen()' and 'Document.webkitExitFullscreen()' instead.";
+        return replacedBy("HTMLVideoElement.webkitExitFullscreen()", "Document.exitFullscreen()' and 'Document.webkitExitFullscreen()");
 
     case PrefixedVideoEnterFullScreen:
-        return "'HTMLVideoElement.webkitEnterFullScreen()' is deprecated. Please use 'Element.requestFullscreen()' and 'Element.webkitRequestFullscreen()' instead.";
+        return replacedBy("HTMLVideoElement.webkitEnterFullScreen()", "Element.requestFullscreen()' and 'Element.webkitRequestFullscreen()");
 
     case PrefixedVideoExitFullScreen:
-        return "'HTMLVideoElement.webkitExitFullScreen()' is deprecated. Please use 'Document.exitFullscreen()' and 'Document.webkitExitFullscreen()' instead.";
+        return replacedBy("HTMLVideoElement.webkitExitFullScreen()", "Document.exitFullscreen()' and 'Document.webkitExitFullscreen()");
 
     case MediaErrorEncrypted:
         return "'MediaError.MEDIA_ERR_ENCRYPTED' is deprecated. This error code is never used.";
 
     case PrefixedGamepad:
         return replacedBy("navigator.webkitGetGamepads", "navigator.getGamepads");
 
     case PrefixedIndexedDB:
         return replacedBy("webkitIndexedDB", "indexedDB");
 
@@ skipping 57 matching lines @@
     case RangeDetach:
         return "'Range.detach' is now a no-op, as per DOM (http://dom.spec.whatwg.org/#dom-range-detach).";
 
     case OverflowChangedEvent:
         return "The 'overflowchanged' event is deprecated and may be removed. Please do not use it.";
 
     case SyncXHRWithCredentials:
         return "Setting 'XMLHttpRequest.withCredentials' for synchronous requests is deprecated.";
 
     case EventSourceURL:
-        return "'EventSource.URL' is deprecated. Please use 'EventSource.url' instead.";
+        return replacedBy("EventSource.URL", "EventSource.url");
 
     case WebSocketURL:
-        return "'WebSocket.URL' is deprecated. Please use 'WebSocket.url' instead.";
+        return replacedBy("WebSocket.URL", "WebSocket.url");
 
     case HTMLTableElementVspace:
-        return "The 'vspace' attribute on table is deprecated. Please use CSS instead.";
+        return replacedBy("The 'vspace' attribute on table", "CSS", false);

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

This should be moved to a more useful suggestion: margin-top and margin-bottom

 
     case HTMLTableElementHspace:
-        return "The 'hspace' attribute on table is deprecated. Please use CSS instead.";
+        return replacedBy("The 'hspace' attribute on table", "CSS", false);

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

Same comment here: margin-left and margin-right

 
     case PictureSourceSrc:
         return "<source src> with a <picture> parent is invalid and therefore ignored. Please use <source srcset> instead.";
 
     case XHRProgressEventPosition:
-        return "The XMLHttpRequest progress event property 'position' is deprecated. Please use 'loaded' instead.";
+        return replacedBy("The XMLHttpRequest progress event property 'position'", "'loaded'", false);
 
     case XHRProgressEventTotalSize:
-        return "The XMLHttpRequest progress event property 'totalSize' is deprecated. Please use 'total' instead.";
+        return replacedBy("The XMLHttpRequest progress event property 'totalSize'", "'total'", false);
 
     case ConsoleTimeline:
-        return "console.timeline is deprecated. Please use the console.time instead.";
+        return replacedBy("console.timeline", "the console.time", false);
 
     case ConsoleTimelineEnd:
-        return "console.timelineEnd is deprecated. Please use the console.timeEnd instead.";
+        return replacedBy("console.timelineEnd", "the console.timeEnd", false);
 
     case XMLHttpRequestSynchronousInNonWorkerOutsideBeforeUnload:
         return "Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check http://xhr.spec.whatwg.org/.";
 
     case FontFaceSetReady:
         return "document.fonts.ready() method is going to be replaced with document.fonts.ready attribute in future releases. Please be prepared. For more help, check https://code.google.com/p/chromium/issues/detail?id=392077#c3 .";
 
     // Features that aren't deprecated don't have a deprecation message.
     default:
         return String();
@@ skipping 35 matching lines @@
 UseCounter* UseCounter::getFrom(const StyleSheetContents* sheetContents)
 {
     // FIXME: We may want to handle stylesheets that have multiple owners
     //        http://crbug.com/242125
     if (sheetContents && sheetContents->hasSingleOwnerNode())
         return getFrom(sheetContents->singleOwnerDocument());
     return 0;
 }
 
 } // namespace blink