Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
455343002:
DevTools: Add new DocumentationURLProvider sources and some tests for documentation (Closed)
|
Created:
6 years, 4 months ago by iliia Modified:
6 years, 4 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionDevTools: Add more JS DocumentationURLProvider sources and some tests for documentation
The following sources are added:
- Node
- Array.prototype
- Date.prototype
- Object.prototype
- String.prototype
BUG=391593
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180066
Patch Set 1 #
Total comments: 11
Patch Set 2 : comments addressed #Patch Set 3 : rebase master #
Total comments: 2
Patch Set 4 : comment addressed #Patch Set 5 : fix test #Patch Set 6 : fix test #
Messages
Total messages: 30 (0 generated)
https://codereview.chromium.org/455343002/diff/1/LayoutTests/inspector/docume... File LayoutTests/inspector/documentation/documentation-url-provider.html (right): https://codereview.chromium.org/455343002/diff/1/LayoutTests/inspector/docume... LayoutTests/inspector/documentation/documentation-url-provider.html:22: ]; 4 spaces left https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:28: ] missing semicolon https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/JSArticle.js:32: if (typeof from["Page_Title"] !== "undefined" && from["Page_Title"] !== {}) from["Page_Title"] !== {} This is always true, thus looks like a bug. https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/JSArticle.js:34: if (typeof from["Standardization_Status"] !== "undefined") you test all these fields for not being "undefined", but in fact you'd like them to be Strings, dont' you? Lets test them for string types then. https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/JSArticle.js:40: if (typeof from["Remarks_Section"] !== "undefined" && typeof from["Remarks_Section"]["Remarks"] !== "undefined") For clarity: if (from["Remarks_Section"] && typeof from["Remarks_Section"]["Remarks"] === "string")
https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/JSArticle.js:34: if (typeof from["Standardization_Status"] !== "undefined") On 2014/08/11 09:42:49, lushnikov wrote: > you test all these fields for not being "undefined", but in fact you'd like them > to be Strings, dont' you? > > Lets test them for string types then. I believe, they will ALWAYS be strings, since this is what parser will provide, no?
Comments addressed; Some changes are in another patch (450323002) https://codereview.chromium.org/455343002/diff/1/LayoutTests/inspector/docume... File LayoutTests/inspector/documentation/documentation-url-provider.html (right): https://codereview.chromium.org/455343002/diff/1/LayoutTests/inspector/docume... LayoutTests/inspector/documentation/documentation-url-provider.html:22: ]; On 2014/08/11 09:42:49, lushnikov wrote: > 4 spaces left Done. https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/DocumentationURLProvider.js (right): https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/DocumentationURLProvider.js:28: ] On 2014/08/11 09:42:49, lushnikov wrote: > missing semicolon Done. https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/JSArticle.js:32: if (typeof from["Page_Title"] !== "undefined" && from["Page_Title"] !== {}) On 2014/08/11 09:42:49, lushnikov wrote: > from["Page_Title"] !== {} > > This is always true, thus looks like a bug. Done. https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/JSArticle.js:34: if (typeof from["Standardization_Status"] !== "undefined") On 2014/08/11 09:42:49, lushnikov wrote: > you test all these fields for not being "undefined", but in fact you'd like them > to be Strings, dont' you? > > Lets test them for string types then. Done. https://codereview.chromium.org/455343002/diff/1/Source/devtools/front_end/do... Source/devtools/front_end/documentation/JSArticle.js:40: if (typeof from["Remarks_Section"] !== "undefined" && typeof from["Remarks_Section"]["Remarks"] !== "undefined") On 2014/08/11 09:42:49, lushnikov wrote: > For clarity: > > if (from["Remarks_Section"] && typeof from["Remarks_Section"]["Remarks"] === > "string") Done.
lgtm
The CQ bit was checked by iliia@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iliia@google.com/455343002/30001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/devtools/front_end/documentation/JSArticle.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/devtools/front_end/documentation/JSArticle.js
Hunk #1 FAILED at 37.
1 out of 1 hunk FAILED -- saving rejects to file
Source/devtools/front_end/documentation/JSArticle.js.rej
Patch: Source/devtools/front_end/documentation/JSArticle.js
Index: Source/devtools/front_end/documentation/JSArticle.js
diff --git a/Source/devtools/front_end/documentation/JSArticle.js
b/Source/devtools/front_end/documentation/JSArticle.js
index
ebaf79855a6285a00eacd5ce85b93062a547ef02..30eebff4fa4dde4baadbc743ecdd9f111ce382a2
100644
--- a/Source/devtools/front_end/documentation/JSArticle.js
+++ b/Source/devtools/front_end/documentation/JSArticle.js
@@ -37,8 +37,10 @@ WebInspector.JSArticle.parse = function(wikiMarkupText)
article.summary = from["Summary_Section"];
if (typeof from["API_Object_Method"] !== "undefined")
article.methods = from["API_Object_Method"];
- if (typeof from["Remarks_Section"] !== "undefined")
- article.remarks = from["Remarks_Section"];
+ if (typeof from["Remarks_Section"] !== "undefined" && typeof
from["Remarks_Section"]["Remarks"] !== "undefined")
+ article.remarks = from["Remarks_Section"]["Remarks"];
+ if (typeof from["Examples_Section"] !== "undefined" && typeof
from["Examples_Section"]["Examples"] !== "undefined")
+ article.examples = from["Examples_Section"]["Examples"];
return article;
}
The CQ bit was checked by iliia@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iliia@google.com/455343002/70001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: 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/2...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19300) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22072)
https://codereview.chromium.org/455343002/diff/70001/Source/devtools/front_en... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/455343002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:43: Please revert this before landing
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)
https://codereview.chromium.org/455343002/diff/70001/Source/devtools/front_en... File Source/devtools/front_end/documentation/JSArticle.js (right): https://codereview.chromium.org/455343002/diff/70001/Source/devtools/front_en... Source/devtools/front_end/documentation/JSArticle.js:43: On 2014/08/11 15:13:35, apavlov wrote: > Please revert this before landing Done.
The CQ bit was checked by iliia@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iliia@google.com/455343002/90001
lgtm
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: 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/2...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19419) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22222)
The CQ bit was checked by iliia@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iliia@google.com/455343002/110001
Not sure why the document parser test has broken, as the parser is not touched in this patch... https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/...
On 2014/08/12 09:48:58, apavlov wrote: > Not sure why the document parser test has broken, as the parser is not touched > in this patch... > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... it comes from previous patch
On 2014/08/12 09:50:55, iliia wrote: > On 2014/08/12 09:48:58, apavlov wrote: > > Not sure why the document parser test has broken, as the parser is not touched > > in this patch... > > > https://storage.googleapis.com/chromium-layout-test-archives/linux_blink_rel/... > > it comes from previous patch Should LayoutTests/inspector/documentation/document-parser-expected.txt be reverted?
The CQ bit was checked by iliia@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/iliia@google.com/455343002/130001
fix test changes
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/19448)
Message was sent while issue was closed.
Change committed as 180066 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
