Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
840493002:
Introduce new test for bookmark loading. (Closed)
|
Created:
5 years, 11 months ago by Alexandre Carlton Modified:
5 years, 11 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@bookmarks-minimal Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce new test for bookmark loading.
BUG=110020
Committed: https://crrev.com/50567ebbd5ca2ff79d87382addc11d25658776d2
Cr-Commit-Position: refs/heads/master@{#312076}
Committed: https://crrev.com/1325b5e80b371e4e559aa745a669c060d08c0ea3
Cr-Commit-Position: refs/heads/master@{#313198}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Implement some of Raymes' changes #Patch Set 3 : Use ScriptingAPI to wait for bookmarks #
Total comments: 10
Patch Set 4 : Implement Raymes' suggestions #Patch Set 5 : Cleanup #
Total comments: 4
Patch Set 6 : Add getter for bookmarks #
Total comments: 2
Patch Set 7 : Fix Documentation #Patch Set 8 : Rebase after failing last time #Patch Set 9 : Rebase for re-upload #Patch Set 10 : Corrections after rebase #
Total comments: 2
Patch Set 11 : Remove newline in argument list. #
Messages
Total messages: 38 (9 generated)
alexandrec@chromium.org changed reviewers: + raymes@chromium.org, sammc@chromium.org
https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:430: }); if we ensure that bookmark information is sent here before the document is loaded, we wouldn't need to add a new event. https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf_extension_test.cc:90: RunTestsInFile("basic_test.js", "/pdf/test.pdf", false); can we just specify the test filename here (e.g. test.pdf rather than /pdf/test.pdf) https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... File chrome/test/data/pdf/bookmarks_test.js (right): https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... chrome/test/data/pdf/bookmarks_test.js:14: nit: here and below, remove a bunch of blink lines https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... chrome/test/data/pdf/bookmarks_test.js:19: chrome.test.assertTrue(firstBookmark.hasOwnProperty('children')); you can probably get rid of all the hasOwnProperty checks - the test will fail if the properties are undefined.
https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:430: }); (we also wouldn't need the bookmarksLoaded member either :) https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf_extension_test.cc:41: void RunTestsInFile(std::string filename, std::string filepath, nit: each param on a separate line
https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf.js:430: }); On 2015/01/13 07:25:12, raymes wrote: > (we also wouldn't need the bookmarksLoaded member either :) Done. https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf_extension_test.cc:41: void RunTestsInFile(std::string filename, std::string filepath, On 2015/01/13 07:25:12, raymes wrote: > nit: each param on a separate line Done. https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf_extension_test.cc:90: RunTestsInFile("basic_test.js", "/pdf/test.pdf", false); On 2015/01/06 05:52:29, raymes wrote: > can we just specify the test filename here (e.g. test.pdf rather than > /pdf/test.pdf) Done. https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... File chrome/test/data/pdf/bookmarks_test.js (right): https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... chrome/test/data/pdf/bookmarks_test.js:14: On 2015/01/06 05:52:29, raymes wrote: > nit: here and below, remove a bunch of blink lines Done. https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... chrome/test/data/pdf/bookmarks_test.js:19: chrome.test.assertTrue(firstBookmark.hasOwnProperty('children')); On 2015/01/06 05:52:29, raymes wrote: > you can probably get rid of all the hasOwnProperty checks - the test will fail > if the properties are undefined. Done.
https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... File chrome/browser/resources/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf_extension_test.cc:41: void RunTestsInFile(std::string filename, std::string filepath, On 2015/01/13 07:25:12, raymes wrote: > nit: each param on a separate line Done. https://codereview.chromium.org/840493002/diff/1/chrome/browser/resources/pdf... chrome/browser/resources/pdf/pdf_extension_test.cc:90: RunTestsInFile("basic_test.js", "/pdf/test.pdf", false); On 2015/01/06 05:52:29, raymes wrote: > can we just specify the test filename here (e.g. test.pdf rather than > /pdf/test.pdf) Done. https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... File chrome/test/data/pdf/bookmarks_test.js (right): https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... chrome/test/data/pdf/bookmarks_test.js:14: On 2015/01/06 05:52:29, raymes wrote: > nit: here and below, remove a bunch of blink lines Done. https://codereview.chromium.org/840493002/diff/1/chrome/test/data/pdf/bookmar... chrome/test/data/pdf/bookmarks_test.js:19: chrome.test.assertTrue(firstBookmark.hasOwnProperty('children')); On 2015/01/06 05:52:29, raymes wrote: > you can probably get rid of all the hasOwnProperty checks - the test will fail > if the properties are undefined. Done.
Looks good! A few minor issues https://codereview.chromium.org/840493002/diff/40001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (left): https://codereview.chromium.org/840493002/diff/40001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:413: console.log(JSON.stringify(this.bookmarks_, null, '\t')); nit: this shouldn't be here. https://codereview.chromium.org/840493002/diff/40001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/840493002/diff/40001/chrome/browser/resources... chrome/browser/resources/pdf/pdf_extension_test.cc:43: bool requiresPlugin) { nit: can you please remove the requiresPlugin param altogether? It shouldn't be needed anymore https://codereview.chromium.org/840493002/diff/40001/chrome/test/data/pdf/boo... File chrome/test/data/pdf/bookmarks_test.js (right): https://codereview.chromium.org/840493002/diff/40001/chrome/test/data/pdf/boo... chrome/test/data/pdf/bookmarks_test.js:8: * Test that the correct bookmarks were loaded for the basic pdf nit: Maybe actually specify the name of the pdf file here nit: add full stop https://codereview.chromium.org/840493002/diff/40001/chrome/test/data/pdf/boo... chrome/test/data/pdf/bookmarks_test.js:13: // Loading all relevant bookmarks nit: add full stop https://codereview.chromium.org/840493002/diff/40001/pdf/out_of_process_insta... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/840493002/diff/40001/pdf/out_of_process_insta... pdf/out_of_process_instance.cc:1096: // TODO(alexandrec): PostMessage the bookmark data from here (to the js). nit do we want this?
https://codereview.chromium.org/840493002/diff/40001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (left): https://codereview.chromium.org/840493002/diff/40001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:413: console.log(JSON.stringify(this.bookmarks_, null, '\t')); On 2015/01/15 05:40:19, raymes wrote: > nit: this shouldn't be here. Done. https://codereview.chromium.org/840493002/diff/40001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/840493002/diff/40001/chrome/browser/resources... chrome/browser/resources/pdf/pdf_extension_test.cc:43: bool requiresPlugin) { On 2015/01/15 05:40:19, raymes wrote: > nit: can you please remove the requiresPlugin param altogether? It shouldn't be > needed anymore Done. https://codereview.chromium.org/840493002/diff/40001/chrome/test/data/pdf/boo... File chrome/test/data/pdf/bookmarks_test.js (right): https://codereview.chromium.org/840493002/diff/40001/chrome/test/data/pdf/boo... chrome/test/data/pdf/bookmarks_test.js:8: * Test that the correct bookmarks were loaded for the basic pdf On 2015/01/15 05:40:19, raymes wrote: > nit: Maybe actually specify the name of the pdf file here > nit: add full stop Done. https://codereview.chromium.org/840493002/diff/40001/chrome/test/data/pdf/boo... chrome/test/data/pdf/bookmarks_test.js:13: // Loading all relevant bookmarks On 2015/01/15 05:40:19, raymes wrote: > nit: add full stop Done. https://codereview.chromium.org/840493002/diff/40001/pdf/out_of_process_insta... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/840493002/diff/40001/pdf/out_of_process_insta... pdf/out_of_process_instance.cc:1096: // TODO(alexandrec): PostMessage the bookmark data from here (to the js). On 2015/01/15 05:40:19, raymes wrote: > nit do we want this? Removed.
https://codereview.chromium.org/840493002/diff/80001/chrome/test/data/pdf/boo... File chrome/test/data/pdf/bookmarks_test.js (right): https://codereview.chromium.org/840493002/diff/80001/chrome/test/data/pdf/boo... chrome/test/data/pdf/bookmarks_test.js:6: nit: we can remove this line https://codereview.chromium.org/840493002/diff/80001/chrome/test/data/pdf/boo... chrome/test/data/pdf/bookmarks_test.js:11: var bookmarks = viewer.bookmarks_; nit: rather than access the private member directly - let's add a getter for it.
https://codereview.chromium.org/840493002/diff/80001/chrome/test/data/pdf/boo... File chrome/test/data/pdf/bookmarks_test.js (right): https://codereview.chromium.org/840493002/diff/80001/chrome/test/data/pdf/boo... chrome/test/data/pdf/bookmarks_test.js:6: On 2015/01/16 02:46:30, raymes wrote: > nit: we can remove this line Done. https://codereview.chromium.org/840493002/diff/80001/chrome/test/data/pdf/boo... chrome/test/data/pdf/bookmarks_test.js:11: var bookmarks = viewer.bookmarks_; On 2015/01/16 02:46:31, raymes wrote: > nit: rather than access the private member directly - let's add a getter for it. Done.
lgtm
https://codereview.chromium.org/840493002/diff/90001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/840493002/diff/90001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:621: * - page Optional?
https://codereview.chromium.org/840493002/diff/90001/chrome/browser/resources... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/840493002/diff/90001/chrome/browser/resources... chrome/browser/resources/pdf/pdf.js:621: * - page On 2015/01/18 23:56:57, Sam McNally wrote: > Optional? Done.
lgtm
The CQ bit was checked by alexandrec@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/840493002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexandrec@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/840493002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexandrec@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/840493002/130001
Message was sent while issue was closed.
Committed patchset #8 (id:130001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/50567ebbd5ca2ff79d87382addc11d25658776d2 Cr-Commit-Position: refs/heads/master@{#312076}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:130001) has been created in https://codereview.chromium.org/857863002/ by alexandrec@chromium.org. The reason for reverting is: Failing on bots http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests%20%28dbg%2....
Preparation for another landing.
lgtm https://codereview.chromium.org/840493002/diff/170001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/840493002/diff/170001/chrome/browser/resource... chrome/browser/resources/pdf/pdf_extension_test.cc:44: std::string pdf_filename) { Will this all fit on a single line?
https://codereview.chromium.org/840493002/diff/170001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/840493002/diff/170001/chrome/browser/resource... chrome/browser/resources/pdf/pdf_extension_test.cc:44: std::string pdf_filename) { On 2015/01/23 00:34:51, raymes wrote: > Will this all fit on a single line? Yes, I'll change it.
The CQ bit was checked by alexandrec@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/840493002/190001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by raymes@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/840493002/190001
Message was sent while issue was closed.
Committed patchset #11 (id:190001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/1325b5e80b371e4e559aa745a669c060d08c0ea3 Cr-Commit-Position: refs/heads/master@{#313198} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
