Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2218883002:
Record HTMLPluginElement::requestObject result. (Closed)
|
Created:
4 years, 4 months ago by mlamouri (slow - plz ping) Modified:
4 years, 4 months ago CC:
asvitkine+watch_chromium.org, blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRecord HTMLPluginElement::requestObject result.
BUG=625984
Committed: https://crrev.com/c91b13c8b1e3eb18efc1e9de4ec67fbb2e678e91
Cr-Commit-Position: refs/heads/master@{#410794}
Patch Set 1 #Patch Set 2 : clean ups #
Total comments: 2
Patch Set 3 : pdr comments #
Messages
Total messages: 31 (18 generated)
Description was changed from ========== Record HTMLPluginElement::requestObject result. BUG=<TODO> ========== to ========== Record HTMLPluginElement::requestObject result. BUG=625984 ==========
mlamouri@chromium.org changed reviewers: + isherman@chromium.org, mkwst@chromium.org
I hope you guys like goto :)
The CQ bit was checked by mlamouri@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: This issue passed the CQ dry run.
-mkwst@ (he is OOO) +foolip@ +asvtikine@ because isherman@ is OOO today
mlamouri@chromium.org changed reviewers: + asvitkine@chromium.org
mlamouri@chromium.org changed reviewers: + foolip@chromium.org - mkwst@chromium.org
asvitkine@chromium.org changed reviewers: + mkwst@chromium.org - foolip@chromium.org
lgtm
mlamouri@chromium.org changed reviewers: - isherman@chromium.org
mlamouri@chromium.org changed reviewers: + foolip@chromium.org - mkwst@chromium.org
+foolip@ for real...
I am sheriff today and tomorrow, can you find another reviewer?
mlamouri@chromium.org changed reviewers: + pdr@chromium.org
pdr@, can you PTAL? :)
https://codereview.chromium.org/2218883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2218883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:460: bool HTMLPlugInElement::requestObject(const String& url, const String& mimeType, const Vector<String>& paramNames, const Vector<String>& paramValues) I don't think gotos are inherently bad but we rarely use them in blink (outside of performance-critical code) so I'd be worried they will cause refactoring bugs in the future. Can you use a private function to avoid using goto? E.g., PluginRequestObjectResult HTMLPluginElement::requestObjectInternal(...) This will also let you avoid the conversions to/from enum/bool since you can write: bool HTMLPlugInElement::requestObject(...) { PluginRequestObjectResult result = requestObjectInternal(...); DEFINE_STATIC_LOCAL(EnumerationHistogram, resultHistogram, ("Plugin.RequestObjectResult", PluginRequestObjectResultMax)); resultHistogram.count(result); if (result == PluginRequestObjectResultFailure) return false; return true; }
PTAL https://codereview.chromium.org/2218883002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (right): https://codereview.chromium.org/2218883002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:460: bool HTMLPlugInElement::requestObject(const String& url, const String& mimeType, const Vector<String>& paramNames, const Vector<String>& paramValues) On 2016/08/09 at 17:45:45, pdr. wrote: > I don't think gotos are inherently bad but we rarely use them in blink (outside of performance-critical code) so I'd be worried they will cause refactoring bugs in the future. Can you use a private function to avoid using goto? > E.g., PluginRequestObjectResult HTMLPluginElement::requestObjectInternal(...) > > This will also let you avoid the conversions to/from enum/bool since you can write: > bool HTMLPlugInElement::requestObject(...) > { > PluginRequestObjectResult result = requestObjectInternal(...); > > DEFINE_STATIC_LOCAL(EnumerationHistogram, resultHistogram, ("Plugin.RequestObjectResult", PluginRequestObjectResultMax)); > resultHistogram.count(result); > > if (result == PluginRequestObjectResultFailure) > return false; > return true; > } I don't think returning PLuginRequestObjectResult really helps because the requestObjectInternal() uses load*() methods which return bool so we would end up doing more conversions. This said, I applied your comments.
The CQ bit was checked by mlamouri@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...
Thanks, LGTM
The CQ bit was unchecked by mlamouri@chromium.org
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2218883002/#ps40001 (title: "pdr comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Record HTMLPluginElement::requestObject result. BUG=625984 ========== to ========== Record HTMLPluginElement::requestObject result. BUG=625984 Committed: https://crrev.com/c91b13c8b1e3eb18efc1e9de4ec67fbb2e678e91 Cr-Commit-Position: refs/heads/master@{#410794} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c91b13c8b1e3eb18efc1e9de4ec67fbb2e678e91 Cr-Commit-Position: refs/heads/master@{#410794} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
