Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2577203002:
Remove use of WebNode/WebElement in translate_helper (Closed)
|
Created:
4 years ago by adithyas Modified:
3 years, 11 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-dom_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, eae+blinkwatch, jam, kinuko+watch, mlamouri+watch-content_chromium.org, rwlbuis, sof Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove use of WebNode/WebElement in translate_helper
Moves DOM crawling code to documentLanguage() and hasMetaValues() to WebDocument and Document
BUG=674714
Review-Url: https://codereview.chromium.org/2577203002
Cr-Commit-Position: refs/heads/master@{#444149}
Committed: https://chromium.googlesource.com/chromium/src/+/5e61528ed7a6cd174f97e14d4cd29b91f8e8baf3
Patch Set 1 #Patch Set 2 : Minor changes #
Total comments: 19
Patch Set 3 : Add WebLanguageDetectionDetails #
Total comments: 3
Patch Set 4 : Move translate specific DOM operations into separate file #
Total comments: 10
Patch Set 5 : Fix nits #Patch Set 6 : Rebase #Patch Set 7 : Rebase again #
Total comments: 1
Messages
Total messages: 30 (15 generated)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Remove use of WebNode/WebElement in translate_helper Moves DOM crawling code to documentLanguage() and hasMetaValues() to WebDocument and Document BUG= ========== to ========== Remove use of WebNode/WebElement in translate_helper Moves DOM crawling code to documentLanguage() and hasMetaValues() to WebDocument and Document BUG=674714 ==========
adithyas@chromium.org changed reviewers: + esprehn@chromium.org, groby@chromium.org, jbroman@chromium.org
https://codereview.chromium.org/2577203002/diff/20001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2577203002/diff/20001/components/translate/co... components/translate/content/renderer/translate_helper.cc:63: // TODO(dglazkov): This logic should be moved into Blink. I think this CL fixes what this comment as looking for, in which case the TODO can be removed. https://codereview.chromium.org/2577203002/diff/20001/components/translate/co... components/translate/content/renderer/translate_helper.cc:65: WebVector<WebString> values = document->getMetaValues(ASCIIToUTF16("google")); super-nit: ASCIIToUTF16 shouldn't be necessary here. I think the converting constructor from char arrays (which takes UTF-8) should be triggered automatically. Ideally we would have an overload of WebString::fromASCII that took a string literal. https://codereview.chromium.org/2577203002/diff/20001/components/translate/co... components/translate/content/renderer/translate_helper.cc:66: for (auto value : values) { nit: this unnecessarily copies WebString (which results in ref count changes); use "const auto&" or "const WebString&". Alternatively, you could use <algorithm>, like so: return std::any_of(values.begin(), values.end(), [](const WebString& value) { return base::LowerCaseEqualsASCII(base::StringPiece16(value), "notranslate"); }); https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1260: if (!htmlElement) { super-nit: You don't need the braces here. https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1273: for (Node* child = headElement->firstChild(); child; You can use the awesome traversal helpers now that you're inside Blink, which trim some boilerplate from this method. for (const HTMLMetaElement& metaElement : Traversal<HTMLMetaElement>::childrenOf(*headElement)) { if (metaElement.name() != name) continue; // and the rest } https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1284: results.push_back(content); super-nit: I believe AtomicString is movable, so you might as well move the existing ref into the array: result.push_back(std::move(content)); https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:375: Vector<AtomicString> getMetaValues(const AtomicString& name) const; Comment (here, and possibly also in WebDocument.h) being more specific about what this does. Something like: // Returns the values of <meta> children of the <head> whose name is a // case-sensitive match for |name|, in order. The standard "content" // attribute is preferred over the non-standard "value". https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebDocument.h (right): https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebDocument.h:79: BLINK_EXPORT WebString documentLanguage() const; Please add comments clarifying the difference between contentLanguage and documentLanguage (here and to Document.h). I can't think of better names, but comments are probably needed. https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebDocument.h:80: BLINK_EXPORT WebVector<WebString> getMetaValues(WebString name) const; prefer "const WebString&" as an argument type
https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1273: for (Node* child = headElement->firstChild(); child; On 2016/12/16 at 16:17:42, jbroman wrote: > You can use the awesome traversal helpers now that you're inside Blink, which trim some boilerplate from this method. > > for (const HTMLMetaElement& metaElement : Traversal<HTMLMetaElement>::childrenOf(*headElement)) { > if (metaElement.name() != name) > continue; > // and the rest > } Or "const auto&" instead of "const HTMLMetaElement&", if you think the line contains the phrase "meta element" too many times already :D
Thanks for jumping into this! \o/ I believe the Blink/content cut you're making is a bit too low. To be more aligned with the Blink Onion soup webmodules thinking, I would prefer moving this logic further into Blink and only leaving a high-level hook. Here's another version of this patch from way earlier: https://codereview.chromium.org/1650303002 Sadly, I didn't have time to finish it.
dglazkov@: I've changed it such that translate_helper uses only a single method in WebDocument to retrieve all the details it needs. https://codereview.chromium.org/2577203002/diff/20001/components/translate/co... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2577203002/diff/20001/components/translate/co... components/translate/content/renderer/translate_helper.cc:63: // TODO(dglazkov): This logic should be moved into Blink. On 2016/12/16 at 16:17:42, jbroman wrote: > I think this CL fixes what this comment as looking for, in which case the TODO can be removed. Done. https://codereview.chromium.org/2577203002/diff/20001/components/translate/co... components/translate/content/renderer/translate_helper.cc:65: WebVector<WebString> values = document->getMetaValues(ASCIIToUTF16("google")); On 2016/12/16 at 16:17:42, jbroman wrote: > super-nit: ASCIIToUTF16 shouldn't be necessary here. I think the converting constructor from char arrays (which takes UTF-8) should be triggered automatically. Ideally we would have an overload of WebString::fromASCII that took a string literal. Okay, changed it. https://codereview.chromium.org/2577203002/diff/20001/components/translate/co... components/translate/content/renderer/translate_helper.cc:66: for (auto value : values) { On 2016/12/16 at 16:17:42, jbroman wrote: > nit: this unnecessarily copies WebString (which results in ref count changes); use "const auto&" or "const WebString&". > > Alternatively, you could use <algorithm>, like so: > > return std::any_of(values.begin(), values.end(), > [](const WebString& value) { > return base::LowerCaseEqualsASCII(base::StringPiece16(value), "notranslate"); > }); Changed to use a const reference and any_of. https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1260: if (!htmlElement) { On 2016/12/16 at 16:17:42, jbroman wrote: > super-nit: You don't need the braces here. Fixed. https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1273: for (Node* child = headElement->firstChild(); child; On 2016/12/16 at 16:17:42, jbroman wrote: > You can use the awesome traversal helpers now that you're inside Blink, which trim some boilerplate from this method. > > for (const HTMLMetaElement& metaElement : Traversal<HTMLMetaElement>::childrenOf(*headElement)) { > if (metaElement.name() != name) > continue; > // and the rest > } Nice, fixed! https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1284: results.push_back(content); On 2016/12/16 at 16:17:42, jbroman wrote: > super-nit: I believe AtomicString is movable, so you might as well move the existing ref into the array: > > result.push_back(std::move(content)); Done. https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.h (right): https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.h:375: Vector<AtomicString> getMetaValues(const AtomicString& name) const; On 2016/12/16 at 16:17:42, jbroman wrote: > Comment (here, and possibly also in WebDocument.h) being more specific about what this does. Something like: > > // Returns the values of <meta> children of the <head> whose name is a > // case-sensitive match for |name|, in order. The standard "content" > // attribute is preferred over the non-standard "value". Done. https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebDocument.h (right): https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebDocument.h:79: BLINK_EXPORT WebString documentLanguage() const; On 2016/12/16 at 16:17:42, jbroman wrote: > Please add comments clarifying the difference between contentLanguage and documentLanguage (here and to Document.h). I can't think of better names, but comments are probably needed. Added comments here and to Document.h. https://codereview.chromium.org/2577203002/diff/20001/third_party/WebKit/publ... third_party/WebKit/public/web/WebDocument.h:80: BLINK_EXPORT WebVector<WebString> getMetaValues(WebString name) const; On 2016/12/16 at 16:17:42, jbroman wrote: > prefer "const WebString&" as an argument type Done.
dglazkov@chromium.org changed reviewers: + dglazkov@chromium.org
https://codereview.chromium.org/2577203002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/Document.cpp (right): https://codereview.chromium.org/2577203002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/Document.cpp:1261: const AtomicString& Document::documentLanguage() const { I would avoid adding a new method on Document. Think of it this way: ideally, the only methods on Document are those that are in Document.idl -- the API description of the class. Everything else is consuming Document as an API. https://codereview.chromium.org/2577203002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebDocument.cpp (right): https://codereview.chromium.org/2577203002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebDocument.cpp:95: WebString WebDocument::documentLanguage() const { Don't need that anymore, right? https://codereview.chromium.org/2577203002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebDocument.cpp:99: WebVector<WebString> WebDocument::getMetaValues(const WebString& name) const { ... Or that.
Very cool, can we introduce a new file that contains the translate related code like getMetaValues and documentLanguage in the .cpp and then call into that for answering the Web* API question. Instead of a method on WebDocument we could add a static function that takes a WebDocument instance and then invokes the internal code?
dglazkov@, esprehn@: I moved the methods I previously defined in Document/WebDocument into WebLanguageDetectionDetails.cpp
lgtm
lgtm just a couple nits https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLanguageDetectionDetails.cpp (right): https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLanguageDetectionDetails.cpp:26: Vector<AtomicString> results; unused variable https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLanguageDetectionDetails.cpp:27: AtomicString google("google"); nit: Since you're making the same AtomicString each time, might as well use DEFINE_STATIC_LOCAL to make it fast: DEFINE_STATIC_LOCAL(const AtomicString, google, ("google")); https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLanguageDetectionDetails.cpp:60: super-nit: the spacing of these lines is a little strange. Is the choice significant, or could this be collapsed? https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebLanguageDetectionDetails.h (right): https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebLanguageDetectionDetails.h:10: #include "public/web/WebDocument.h" nit: Technically you just need a forward declaration here. You can remove this include and put "class WebDocument;" inside the Blink namespace. https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebLanguageDetectionDetails.h:18: bool hasNoTranslateMeta; nit: Rather than writing an explicit constructor, you can just write "bool hasNoTranslateMeta = false;" here.
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebLanguageDetectionDetails.cpp (right): https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLanguageDetectionDetails.cpp:26: Vector<AtomicString> results; On 2016/12/20 at 16:43:51, jbroman wrote: > unused variable Removed. https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLanguageDetectionDetails.cpp:27: AtomicString google("google"); On 2016/12/20 at 16:43:51, jbroman wrote: > nit: Since you're making the same AtomicString each time, might as well use DEFINE_STATIC_LOCAL to make it fast: > > DEFINE_STATIC_LOCAL(const AtomicString, google, ("google")); Done. https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebLanguageDetectionDetails.cpp:60: On 2016/12/20 at 16:43:51, jbroman wrote: > super-nit: the spacing of these lines is a little strange. Is the choice significant, or could this be collapsed? Fixed :) https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/web/WebLanguageDetectionDetails.h (right): https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebLanguageDetectionDetails.h:10: #include "public/web/WebDocument.h" On 2016/12/20 at 16:43:51, jbroman wrote: > nit: Technically you just need a forward declaration here. You can remove this include and put "class WebDocument;" inside the Blink namespace. Done. https://codereview.chromium.org/2577203002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/web/WebLanguageDetectionDetails.h:18: bool hasNoTranslateMeta; On 2016/12/20 at 16:43:51, jbroman wrote: > nit: Rather than writing an explicit constructor, you can just write "bool hasNoTranslateMeta = false;" here. Removed the constructor.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
groby@: could you please take a look at this patch? :)
lgtm https://codereview.chromium.org/2577203002/diff/120001/components/translate/c... File components/translate/content/renderer/translate_helper.cc (right): https://codereview.chromium.org/2577203002/diff/120001/components/translate/c... components/translate/content/renderer/translate_helper.cc:102: std::string content_language = web_detection_details.contentLanguage.utf8(); Sidebar: It's sad we need to jump through conversion hoops. Will Blink and Chromium ever share a string type? (Not relevant to CL, just curious)
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org, dglazkov@chromium.org Link to the patchset: https://codereview.chromium.org/2577203002/#ps120001 (title: "Rebase again")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1484682850684750,
"parent_rev": "ad0eb0fa0ba570b4e41e28383dda35d1aca0a894", "commit_rev":
"5e61528ed7a6cd174f97e14d4cd29b91f8e8baf3"}
Message was sent while issue was closed.
Description was changed from ========== Remove use of WebNode/WebElement in translate_helper Moves DOM crawling code to documentLanguage() and hasMetaValues() to WebDocument and Document BUG=674714 ========== to ========== Remove use of WebNode/WebElement in translate_helper Moves DOM crawling code to documentLanguage() and hasMetaValues() to WebDocument and Document BUG=674714 Review-Url: https://codereview.chromium.org/2577203002 Cr-Commit-Position: refs/heads/master@{#444149} Committed: https://chromium.googlesource.com/chromium/src/+/5e61528ed7a6cd174f97e14d4cd2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/5e61528ed7a6cd174f97e14d4cd2... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
