Summary: | Fix inspector override conversion in InspectorPageAgent::overrideSetting | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, hi, joepeck, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=199924 | ||||||||
Attachments: |
|
Description
youenn fablet
2019-07-22 15:26:57 PDT
Created attachment 374643 [details]
Patch
Comment on attachment 374643 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=374643&action=review r=me, nice catch! > Source/WebCore/ChangeLog:15 > + Previously, the conversion for the mock capture value was implicit from a bool pointer to an optional. > + Make an explicit conversion as done for regular settings. Style: there seems to be an extra space before each of these lines. > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:429 > + return *value; Style: I prefer using `.value()` with `WTF::Optional` so it's a bit clearer that it's not a pointer. > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:445 > + auto overrideValue = toOptionalBool(value); NIT: Web Inspector typically uses `as*` instead of `to*` for it's type conversions (see `asBool` in InspectorRuntimeAgent.cpp). > Source/WebCore/inspector/agents/InspectorPageAgent.cpp:448 > + case Inspector::Protocol::Page::Setting::name: { \ NIT: we can remove the `{` and `}` and make this slightly simpler. Created attachment 374649 [details]
Patch for landing
Done.
> Style: I prefer using `.value()` with `WTF::Optional` so it's a bit clearer
> that it's not a pointer.
I like using pointer-like code, for instance "if(value) value->doSomething()" or "if(value) doSomething(*value)".
The one case I am sometimes using .value() is when moving the value.
"if (value) doSomething(WTFMove(*value))" seems a bit strange.
Comment on attachment 374649 [details] Patch for landing Clearing flags on attachment: 374649 Committed r247707: <https://trac.webkit.org/changeset/247707> All reviewed patches have been landed. Closing bug. |