DocumentationURLProvider class created

DevTools: DocumentationURLProvider class created

This class helps DocumentationView to form a URL to download documentation.

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

[lushnikov, 2014-08-06 11:25:18]

Please add test as per offline discussion

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
File Source/devtools/front_end/documentation/DocumentationURLFormer.js (right):

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:1: /**
LICENSE is missing

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:4:
WebInspector.DocumentationURLFormer = function() {}
DocumentationURLProvider?

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:13: for (var i
= 0; i < this._Sources.length; i++) {
Our codestyle requires ++i

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:17: return
"http://docs.webplatform.org/w/api.php?action=query&titles=" + url
use String.sprintf

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:20: return
"NONE";
return null;

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:23: _Sources:
[
This should be a field on WebInspector.DocumentationURLFormer

[yurys, 2014-08-06 11:37:05]

In addtition to Andrey's comments - this file should be added to appropriate
.gyp/gypi files and module descriptors.

[semeny, 2014-08-06 17:17:30]

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
File Source/devtools/front_end/documentation/DocumentationURLFormer.js (right):

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:1: /**
On 2014/08/06 11:25:18, lushnikov wrote:
> LICENSE is missing

Done.

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:4:
WebInspector.DocumentationURLFormer = function() {}
On 2014/08/06 11:25:18, lushnikov wrote:
> DocumentationURLProvider?

Done.

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:13: for (var i
= 0; i < this._Sources.length; i++) {
On 2014/08/06 11:25:17, lushnikov wrote:
> Our codestyle requires ++i

Done.

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:17: return
"http://docs.webplatform.org/w/api.php?action=query&titles=" + url
On 2014/08/06 11:25:18, lushnikov wrote:
> use String.sprintf

Done.

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:20: return
"NONE";
On 2014/08/06 11:25:17, lushnikov wrote:
> return null;

Done.

https://codereview.chromium.org/447703002/diff/1/Source/devtools/front_end/do...
Source/devtools/front_end/documentation/DocumentationURLFormer.js:23: _Sources:
[
On 2014/08/06 11:25:17, lushnikov wrote:
> This should be a field on WebInspector.DocumentationURLFormer

Done.

[lushnikov, 2014-08-06 17:27:52]

Overall looks good! A few more comments.

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
File LayoutTests/inspector/documentation-url-provider-expected.txt (right):

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider-expected.txt:2:
http://docs.webplatform.org/w/api.php?action=query&titles=javascript/Math/max...
did you try all these URL's in life? All work fine?

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
File LayoutTests/inspector/documentation-url-provider.html (right):

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:17: "fromCharCode",
These are all methods that exist. Let's add some that don't and verify that
DocumentationURLProvider doesn't crash in this case.

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:18: "keys"];
"];" on a new line

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:19: for (var i = 0; i <
testCases.length; i++) {
++i

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:20: var urlFormer = new
WebInspector.DocumentationURLProvider();
do we really need to recreate it for every for-loop iteration?

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:29: <body
onload="runTest()">
We usually add a short test description in <body> tag. (see examples in
different tests)

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/documentation/DocumentationURLProvider.js
(right):

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/documentation/DocumentationURLProvider.js:11: {source:
window, url: "javascript/"},
space after "{" and before "}"

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/documentation/DocumentationURLProvider.js:31: return
String.sprintf("http://docs.webplatform.org/w/api.php?action=query&titles=%s%s&prop=revisions&rvprop=timestamp|content&format=json",
extract to constant

[yurys, 2014-08-07 08:27:37]

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/documentation/DocumentationURLProvider.js
(right):

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/documentation/DocumentationURLProvider.js:10:
WebInspector.DocumentationURLProvider._Sources = [
Please add type annotation. Also it might make sense to defined a type for the
elements.

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/documentation/DocumentationURLProvider.js:26:
formItemPath: function(searchTerm)
Just itemPath

[semeny, 2014-08-07 09:57:08]

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
File LayoutTests/inspector/documentation-url-provider.html (right):

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:17: "fromCharCode",
On 2014/08/06 17:27:51, lushnikov wrote:
> These are all methods that exist. Let's add some that don't and verify that
> DocumentationURLProvider doesn't crash in this case.

Done.

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:18: "keys"];
On 2014/08/06 17:27:51, lushnikov wrote:
> "];" on a new line

Done.

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:19: for (var i = 0; i <
testCases.length; i++) {
On 2014/08/06 17:27:51, lushnikov wrote:
> ++i

Done.

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:20: var urlFormer = new
WebInspector.DocumentationURLProvider();
On 2014/08/06 17:27:51, lushnikov wrote:
> do we really need to recreate it for every for-loop iteration?

Done.

https://codereview.chromium.org/447703002/diff/40001/LayoutTests/inspector/do...
LayoutTests/inspector/documentation-url-provider.html:29: <body
onload="runTest()">
On 2014/08/06 17:27:51, lushnikov wrote:
> We usually add a short test description in <body> tag. (see examples in
> different tests)

Done.

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
File Source/devtools/front_end/documentation/DocumentationURLProvider.js
(right):

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/documentation/DocumentationURLProvider.js:10:
WebInspector.DocumentationURLProvider._Sources = [
On 2014/08/07 08:27:37, yurys wrote:
> Please add type annotation. Also it might make sense to defined a type for the
> elements.

Done.

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/documentation/DocumentationURLProvider.js:11: {source:
window, url: "javascript/"},
On 2014/08/06 17:27:51, lushnikov wrote:
> space after "{" and before "}"

Done.

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/documentation/DocumentationURLProvider.js:26:
formItemPath: function(searchTerm)
On 2014/08/07 08:27:37, yurys wrote:
> Just itemPath

Done.

https://codereview.chromium.org/447703002/diff/40001/Source/devtools/front_en...
Source/devtools/front_end/documentation/DocumentationURLProvider.js:31: return
String.sprintf("http://docs.webplatform.org/w/api.php?action=query&titles=%s%s&prop=revisions&rvprop=timestamp|content&format=json",
On 2014/08/06 17:27:51, lushnikov wrote:
> extract to constant

Done.

[lushnikov, 2014-08-07 10:45:54]

Let's move test to inspector/documentation

Otherwise lgtm

[iliia, 2014-08-08 07:10:09]

The CQ bit was checked by iliia@google.com

[commit-bot: I haz the power, 2014-08-08 07:11:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/semeny@google.com/447703002/160001

[commit-bot: I haz the power, 2014-08-08 07:32:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_db...)
  android_blink_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_blink_compile_re...)
  android_chromium_gn_compile_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/android_chromium_gn_comp...)
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/2...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  linux_chromium_gn_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/18959)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21692)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/39002)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/44234)

[commit-bot: I haz the power, 2014-08-08 07:33:45]

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

[commit-bot: I haz the power, 2014-08-08 07:33:46]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/39016)

[semeny, 2014-08-08 11:18:11]

The CQ bit was checked by semeny@google.com

[commit-bot: I haz the power, 2014-08-08 11:18:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/semeny@google.com/447703002/200001

[yurys, 2014-08-08 12:17:07]

lgtm with comment

https://codereview.chromium.org/447703002/diff/200001/Source/devtools/scripts...
File Source/devtools/scripts/frontend_modules.json (right):

https://codereview.chromium.org/447703002/diff/200001/Source/devtools/scripts...
Source/devtools/scripts/frontend_modules.json:505:
"documentation/DocumentationView.js",
Please sort the sources alphabetically.

[yurys, 2014-08-08 12:18:08]

https://codereview.chromium.org/447703002/diff/200001/Source/devtools/devtool...
File Source/devtools/devtools.gypi (right):

https://codereview.chromium.org/447703002/diff/200001/Source/devtools/devtool...
Source/devtools/devtools.gypi:443:
'front_end/documentation/DocumentationView.js',
This files should also be sorted.

[semeny, 2014-08-08 12:22:59]

The CQ bit was unchecked by semeny@google.com

[semeny, 2014-08-08 12:28:02]

The CQ bit was checked by semeny@google.com

[commit-bot: I haz the power, 2014-08-08 12:28:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/semeny@google.com/447703002/220001

[commit-bot: I haz the power, 2014-08-08 13:42:49]

Change committed as 179831