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

Issue 1601873002: Adds a name function for constraints (Closed)

Created:
4 years, 11 months ago by hta - Chromium
Modified:
4 years, 11 months ago
CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, kinuko+watch, mlamouri+watch-blink_chromium.org, philipj_slow, tommyw+watchlist_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Adds a name function for constraints. The constraints specification requires that one return the name of a failing constraint in an error message. It's therefore required to be able to extract the name to return from the constraint. This CL adds that function. BUG=543997 Committed: https://crrev.com/1bb1840db5ac2d6af5bd73ee2b498bc28867c095 Cr-Commit-Position: refs/heads/master@{#370378}

Patch Set 1 #

Patch Set 2 : Adds name() function to constraints #

Total comments: 5

Patch Set 3 : Fixed unintentional pass-by-values. #

Total comments: 3

Patch Set 4 : Review comment fixes #

Unified diffs Side-by-side diffs Delta from patch set Stats (+169 lines, -67 lines) Patch
M third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp View 1 2 3 chunks +6 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/modules/mediastream/MediaConstraintsTest.cpp View 1 5 chunks +12 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/WebMediaConstraints.cpp View 1 5 chunks +107 lines, -1 line 0 comments Download
M third_party/WebKit/public/platform/WebMediaConstraints.h View 1 2 3 8 chunks +44 lines, -52 lines 0 comments Download

Messages

Total messages: 21 (9 generated)
hta - Chromium
This CL also introduces a virtual base class for constraints. Comments on the design choices ...
4 years, 11 months ago (2016-01-19 08:58:56 UTC) #3
tommi (sloooow) - chröme
design lgtm. Would like to fix these by value cases below, assuming they're not intentional. ...
4 years, 11 months ago (2016-01-19 09:14:19 UTC) #4
hta - Chromium
Solved one, couldn't see how to solve the other. https://codereview.chromium.org/1601873002/diff/20001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1601873002/diff/20001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode428 third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:428: ...
4 years, 11 months ago (2016-01-19 11:14:07 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/1601873002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601873002/40001
4 years, 11 months ago (2016-01-19 11:14:49 UTC) #7
tommi (sloooow) - chröme
thanks lgtm. https://codereview.chromium.org/1601873002/diff/20001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp File third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp (right): https://codereview.chromium.org/1601873002/diff/20001/third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp#newcode431 third_party/WebKit/Source/modules/mediastream/MediaConstraintsImpl.cpp:431: webForm.setIdeal(WebVector<WebString>(blinkForm.ideal())); On 2016/01/19 11:14:07, hta - Chromium ...
4 years, 11 months ago (2016-01-19 11:24:34 UTC) #8
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-19 12:28:22 UTC) #10
jochen (gone - plz use gerrit)
can you please update the CL description to by <80c and have a one-line summary ...
4 years, 11 months ago (2016-01-19 17:31:23 UTC) #11
hta - Chromium
Review comments fixed.PTAL.
4 years, 11 months ago (2016-01-20 09:33:08 UTC) #13
jochen (gone - plz use gerrit)
lgtm
4 years, 11 months ago (2016-01-20 12:31:41 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1601873002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1601873002/60001
4 years, 11 months ago (2016-01-20 12:41:59 UTC) #17
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 11 months ago (2016-01-20 13:45:51 UTC) #19
commit-bot: I haz the power
4 years, 11 months ago (2016-01-20 13:47:29 UTC) #21
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/1bb1840db5ac2d6af5bd73ee2b498bc28867c095
Cr-Commit-Position: refs/heads/master@{#370378}

Powered by Google App Engine
This is Rietveld 408576698