Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(334)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1730383003: DevTools: consistently use Maybe for optional values in the protocol generator. (Closed)

Created:
4 years, 10 months ago by pfeldman
Modified:
4 years, 9 months ago
Reviewers:
Nico, dgozman
CC:
chromium-reviews, caseq+blink_chromium.org, cmumford, aboxhall, aboxhall+watch_chromium.org, nektar+watch_chromium.org, lushnikov+blink_chromium.org, yuzo+watch_chromium.org, pfeldman+blink_chromium.org, nektarios, dmazzoni, apavlov+blink_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, devtools-reviews_chromium.org, blink-reviews, je_julie, sergeyv+blink_chromium.org, jsbell+idb_chromium.org, kinuko+watch, kozyatinskiy+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

DevTools: consistently use Maybe for optional values in the protocol generator. BUG=580337 Committed: https://crrev.com/794157c8a8848808c00194d820bf0a210550e914 Cr-Commit-Position: refs/heads/master@{#377505}

Patch Set 1 #

Patch Set 2 : more exports #

Total comments: 14

Patch Set 3 : review comments addressed #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+524 lines, -552 lines) Patch
M third_party/WebKit/Source/core/inspector/InspectorBaseAgent.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorCSSAgent.h View 2 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp View 1 7 chunks +7 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorDOMAgent.h View 3 chunks +14 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorDOMAgent.cpp View 17 chunks +50 lines, -48 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.h View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorDOMDebuggerAgent.cpp View 1 2 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorDebuggerAgent.h View 1 chunk +12 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorDebuggerAgent.cpp View 11 chunks +32 lines, -29 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorHeapProfilerAgent.h View 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorHeapProfilerAgent.cpp View 2 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorInputAgent.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorInputAgent.cpp View 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.h View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorLayerTreeAgent.cpp View 2 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorPageAgent.h View 2 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorPageAgent.cpp View 4 chunks +8 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorResourceAgent.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorRuntimeAgent.h View 3 chunks +6 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorRuntimeAgent.cpp View 5 chunks +29 lines, -29 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorTracingAgent.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorTracingAgent.cpp View 1 chunk +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/modules/accessibility/InspectorAccessibilityAgent.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/accessibility/InspectorTypeBuilderHelper.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/InspectorIndexedDBAgent.h View 1 chunk +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/indexeddb/InspectorIndexedDBAgent.cpp View 1 2 3 chunks +12 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/modules/webdatabase/InspectorDatabaseAgent.cpp View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/inspector_protocol/CodeGenerator.py View 6 chunks +0 lines, -24 lines 0 comments Download
M third_party/WebKit/Source/platform/inspector_protocol/Dispatcher_cpp.template View 9 chunks +22 lines, -23 lines 0 comments Download
M third_party/WebKit/Source/platform/inspector_protocol/Dispatcher_h.template View 3 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/inspector_protocol/Frontend_cpp.template View 2 chunks +3 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/platform/inspector_protocol/Frontend_h.template View 1 chunk +1 line, -6 lines 0 comments Download
M third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_cpp.template View 1 chunk +4 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template View 1 2 4 chunks +81 lines, -85 lines 2 comments Download
M third_party/WebKit/Source/platform/v8_inspector/InjectedScript.h View 5 chunks +12 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/InjectedScript.cpp View 12 chunks +17 lines, -17 lines 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.h View 3 chunks +28 lines, -28 lines 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/V8DebuggerAgentImpl.cpp View 1 17 chunks +48 lines, -48 lines 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/V8DebuggerImpl.cpp View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/V8HeapProfilerAgentImpl.h View 2 chunks +5 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/V8HeapProfilerAgentImpl.cpp View 6 chunks +8 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/V8RuntimeAgentImpl.h View 3 chunks +25 lines, -25 lines 0 comments Download
M third_party/WebKit/Source/platform/v8_inspector/V8RuntimeAgentImpl.cpp View 8 chunks +37 lines, -37 lines 0 comments Download
M third_party/WebKit/Source/web/InspectorEmulationAgent.h View 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/web/InspectorEmulationAgent.cpp View 3 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 22 (6 generated)
pfeldman
4 years, 10 months ago (2016-02-24 20:22:40 UTC) #2
pfeldman
TypeBuilder and CodeGenerator are the ones to look at.
4 years, 10 months ago (2016-02-24 20:45:43 UTC) #3
dgozman
https://codereview.chromium.org/1730383003/diff/20001/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1730383003/diff/20001/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp#newcode914 third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:914: pseudoIdMatches->fromJust()->addItem(protocol::CSS::PseudoElementMatches::create() Can we have operator -> on Maybe<OwnPtr<T>> and ...
4 years, 10 months ago (2016-02-24 23:17:44 UTC) #4
pfeldman
https://codereview.chromium.org/1730383003/diff/20001/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp File third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp (right): https://codereview.chromium.org/1730383003/diff/20001/third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp#newcode914 third_party/WebKit/Source/core/inspector/InspectorCSSAgent.cpp:914: pseudoIdMatches->fromJust()->addItem(protocol::CSS::PseudoElementMatches::create() On 2016/02/24 23:17:43, dgozman wrote: > Can we ...
4 years, 10 months ago (2016-02-24 23:38:39 UTC) #5
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1730383003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1730383003/40001
4 years, 10 months ago (2016-02-24 23:54:18 UTC) #7
dgozman
lgtm
4 years, 10 months ago (2016-02-24 23:57:08 UTC) #8
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/179130)
4 years, 10 months ago (2016-02-25 02:44:02 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1730383003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1730383003/40001
4 years, 10 months ago (2016-02-25 03:21:10 UTC) #12
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 10 months ago (2016-02-25 04:29:27 UTC) #13
commit-bot: I haz the power
Patchset 3 (id:??) landed as https://crrev.com/794157c8a8848808c00194d820bf0a210550e914 Cr-Commit-Position: refs/heads/master@{#377505}
4 years, 10 months ago (2016-02-25 04:31:01 UTC) #15
Nico
https://codereview.chromium.org/1730383003/diff/40001/third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template File third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template (right): https://codereview.chromium.org/1730383003/diff/40001/third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template#newcode42 third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template:42: bool isJust() const { return m_isJust; } isJust() isn't ...
4 years, 10 months ago (2016-02-26 19:14:06 UTC) #17
Nico
https://codereview.chromium.org/1730383003/diff/40001/third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template File third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template (right): https://codereview.chromium.org/1730383003/diff/40001/third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template#newcode31 third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template:31: OwnPtr<T> m_value; also, i found it pretty surprising that ...
4 years, 10 months ago (2016-02-26 19:31:48 UTC) #18
pfeldman
On 2016/02/26 19:14:06, Nico wrote: > https://codereview.chromium.org/1730383003/diff/40001/third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template > File > third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template > (right): > > ...
4 years, 9 months ago (2016-03-01 00:18:09 UTC) #19
pfeldman
wtf
4 years, 9 months ago (2016-03-01 00:24:09 UTC) #20
Nico
I complained on the v8 review that changed v8s version from hasValue to isJust too, ...
4 years, 9 months ago (2016-03-01 00:28:15 UTC) #21
Nico
4 years, 9 months ago (2016-03-01 00:28:19 UTC) #22
Message was sent while issue was closed. 
I complained on the v8 review that changed v8s version from hasValue to
isJust too, but both author and reviewer of that patch have left :-/ I'll
try to get this renamed back in v8, thanks.
On Feb 29, 2016 4:18 PM, <pfeldman@chromium.org> wrote:

> On 2016/02/26 19:14:06, Nico wrote:
> >
>
>
https://codereview.chromium.org/1730383003/diff/40001/third_party/WebKit/Sour...
> > File
> >
> third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template
> > (right):
> >
> >
>
>
https://codereview.chromium.org/1730383003/diff/40001/third_party/WebKit/Sour...
> >
>
>
third_party/WebKit/Source/platform/inspector_protocol/TypeBuilder_h.template:42:
> > bool isJust() const { return m_isJust; }
> > isJust() isn't a great name for this. People who know Haskell will get
> this,
> > nobody else will without looking it up.
>
> This is named to be consistent with
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/v8/include/v8.h&q=...
> since all inspector-protocol versions are working against v8-based
> targets. But
> I agree with you, we might go to Optional. Unfortunately, it clashes with
> the
> wft.
>
> https://codereview.chromium.org/1730383003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698