[clang-tidy] Fix access via null pointer to objects [chromium/src : master]

0 views

Skip to first unread message

Zinovy Nis (Gerrit)

unread,

Mar 9, 2018, 11:46:19 AM3/9/18

to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, Peter Kasting, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

This change is ready for review.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Alexei Svitkine (Gerrit)

unread,

Mar 9, 2018, 12:06:01 PM3/9/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

If we don't have crashes in code below that does ptr->..., perhaps we should just delete the null checks instead? Maybe replacing with DCHECKs()?

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 9, 2018, 1:12:30 PM3/9/18

to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, Peter Kasting, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 1:

If we don't have crashes in code below that does ptr->..., perhaps we should just delete the null checks instead? Maybe replacing with DCHECKs()?

Not sure about not having crashes.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Alexei Svitkine

unread,

Mar 9, 2018, 1:26:34 PM3/9/18

to change...@chromium-review.googlesource.com, asvitkine+watch, blink-re...@chromium.org, blink-reviews, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Chrome has crash reporting and crashes are routinely addressed. So I think given crash reporting has not alerted us to crashes in these places, it's safe to assume that these pointers cannot be null currently.

Alexei Svitkine (Gerrit)

unread,

Mar 9, 2018, 1:26:45 PM3/9/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Chrome has crash reporting and crashes are routinely addressed. So I think given crash reporting has not alerted us to crashes in these places, it's safe to assume that these pointers cannot be null currently.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Peter Kasting (Gerrit)

unread,

Mar 9, 2018, 7:12:28 PM3/9/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

6 comments:

File base/metrics/field_trial.cc:
- Patch Set #1, Line 1447: if (allocator->IsReadonly()))
  Nit: Simpler:
```
  if (!allocator || allocator->IsReadOnly())
```
  I don't know this code, so I can't tell you whether |allocator| can actually be null. AddToAllocatorWhileLocked() handles the case of a null allocator, which supports the idea that it can be.
- Patch Set #1, Line 1454: AddToAllocatorWhileLocked(global_->field_trial_allocator_.get(),
  Nit: While here: change the first arg of this to |allocator| (since that's what it is)
File chrome/browser/search/search.cc:
- Patch Set #1, Line 382: if (!profile)
  I think |profile| cannot be null here, so I'd remove this check.
File chrome/browser/ui/browser_commands.cc:
- Patch Set #1, Line 876: if (!current_tab)
  Various other commands in this file assume an active web contents. I think it's safe to remove this check.
File content/browser/renderer_host/render_widget_host_impl.cc:
- Patch Set #1, Line 1712: if (!delegate_)
  Functions like OnStartDragging() assume a delegate, so I think it's OK to remove this check.
File third_party/WebKit/Source/core/dom/Document.cpp:
- Patch Set #1, Line 2332: if (!frame_view) {
  Nit: Change to:
```
  DCHECK(frame_view);
  DCHECK(!frame_view->IsInPerformLayout()) << "View Layout should not be re-entrant";
```
  (The old code violated the style guide by handling DCHECK failure)

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 10, 2018, 9:49:53 AM3/10/18

to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Alexei Svitkine, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 1:
(6 comments)

Thanks! Updated the patch.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 11, 2018, 2:38:05 PM3/11/18

to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Alexei Svitkine, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 2:

Patch Set 1:
(6 comments)
Thanks! Updated the patch.

Gentle ping for this trivial patch.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Peter Kasting (Gerrit)

unread,

Mar 11, 2018, 3:18:35 PM3/11/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 2:
Patch Set 2:
Patch Set 1:
(6 comments)
Thanks! Updated the patch.
Gentle ping for this trivial patch.

Note that it's the weekend and most of us aren't working -- will take a look at your patch when back in the office.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Alexei Svitkine (Gerrit)

unread,

Mar 12, 2018, 1:05:20 PM3/12/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

LGTM % comment

Patch set 2:Code-Review +1

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:
- Patch Set #2, Line 2334:
  Nit: I think putting the message as a comment above the DCHECK() is better, since then the extra text doesn't get compiled into builds and bloat the binary or compile/link times - while still available when someone goes to investigate what's wrong.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Peter Kasting (Gerrit)

unread,

Mar 12, 2018, 2:05:27 PM3/12/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 2:Code-Review +1

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #2, Line 2334:

- Nit: I think putting the message as a comment above the DCHECK() is better, since then the extra tex […]
  DCHECK << "" shouldn't cause release-mode binary bloat, should it? If it does it seems like we have larger problems.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 12, 2018, 4:27:38 PM3/12/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, Peter Kasting, Alexei Svitkine, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 2: Code-Review+1
(1 comment)

Alexei and Peter, thanks for LGTMs!
I added few more similar fixes not to bloat additional reviews. Hope you don't mind.

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #2, Line 2334:

- DCHECK << "" shouldn't cause release-mode binary bloat, should it? If it does it seems like we have […]
  There a lot of uses of <DCHECK() << "String"> in Chromium code and I don't think it poses any perf issue.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Alexei Svitkine (Gerrit)

unread,

Mar 12, 2018, 4:29:50 PM3/12/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, Peter Kasting, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #2, Line 2334:
  DCHECK << "" shouldn't cause release-mode binary bloat, should it? If it does it seems like we have […]

- That's correct. But it does for debug binaries (which will slow down compiles, bots, etc).
  And the benefit don't seem worth the bloat there given whoever investigates this will look at this code.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Peter Kasting (Gerrit)

unread,

Mar 12, 2018, 4:35:34 PM3/12/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #2, Line 2334:

- That's correct. But it does for debug binaries (which will slow down compiles, bots, etc). […]
  I don't think we should discourage people from adding log messages on failed DCHECKs; in fact I'd prefer more of our comments became log messages. This would often help track down failures on bots faster.
  I don't think there's significant impact on compile speed/bot runtime from this. Binary size can take an aggregate hit, but for debug, it's not so severe as to cause problems we care about.
  If you still disagree, I suggest raising this with a larger audience (e.g. chromium-dev) to see if there's more consensus that reviewers should discourage explanatory messages on DCHECKs.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 14, 2018, 1:11:07 AM3/14/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Commit Bot, Peter Kasting, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 6:Commit-Queue +2

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 14, 2018, 1:11:12 AM3/14/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"[clang-tidy] Fix access via null pointer to objects" https://chromium-review.googlesource.com/c/955852/6

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/955852/6

Bot data: {"action": "start", "triggered_at": "2018-03-14T05:11:00.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "326006ec2417cded71e1b536d71d9180b3460481"}

Commit Bot (Gerrit)

unread,

Mar 14, 2018, 1:11:13 AM3/14/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

only full committers or CL owner with tryjob access are allowed to trigger CQ

Zinovy Nis (Gerrit)

unread,

Mar 14, 2018, 1:13:41 AM3/14/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Commit Bot, Peter Kasting, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #2, Line 2334:
  That's correct. But it does for debug binaries (which will slow down compiles, bots, etc). […]

- Well, I restored the text message passed to DCHECK stream. Please have a look at new version.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 14, 2018, 6:56:35 AM3/14/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Commit Bot, Peter Kasting, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 6:Commit-Queue +2

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 14, 2018, 6:56:38 AM3/14/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"[clang-tidy] Fix access via null pointer to objects" https://chromium-review.googlesource.com/c/955852/6

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/955852/6

Bot data: {"action": "start", "triggered_at": "2018-03-14T10:56:32.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "326006ec2417cded71e1b536d71d9180b3460481"}

Commit Bot (Gerrit)

unread,

Mar 14, 2018, 6:56:39 AM3/14/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

only full committers or CL owner with tryjob access are allowed to trigger CQ

Alexei Svitkine (Gerrit)

unread,

Mar 14, 2018, 11:06:03 AM3/14/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 6:Commit-Queue +2

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 14, 2018, 11:06:06 AM3/14/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.
"[clang-tidy] Fix access via null pointer to objects" https://chromium-review.googlesource.com/c/955852/6

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/955852/6

Bot data: {"action": "start", "triggered_at": "2018-03-14T15:06:01.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "326006ec2417cded71e1b536d71d9180b3460481"}

Commit Bot (Gerrit)

unread,

Mar 14, 2018, 11:12:24 AM3/14/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Try jobs failed on following builders:
  chromium_presubmit on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/chromium_presubmit/57593)

Zinovy Nis (Gerrit)

unread,

Mar 14, 2018, 11:20:27 AM3/14/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 6: Commit-Queue+2

No LGTM from owners of some files yet ((

Zinovy Nis (Gerrit)

unread,

Mar 14, 2018, 1:36:28 PM3/14/18

to Jochen Eisinger, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Alexei Svitkine, Peter Kasting

Zinovy Nis would like Jochen Eisinger to review this change.

[clang-tidy] Fix access via null pointer to objects

Found with clang-tidy [clang-analyzer-core.CallAndMessage].
Fixed few cases with an incorrect access pattern:

if (ptr && ptr->HasSomeFeature())
  return;

ptr->...  // ptr here can be null!

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I004c92a6335c9d9c0c7fa14b7e8e83279cb431e6
---
M base/metrics/field_trial.cc
M chrome/browser/extensions/api/tabs/tabs_api.cc
M chrome/browser/search/search.cc
M chrome/browser/ui/browser_commands.cc
M content/browser/renderer_host/render_widget_host_impl.cc
M extensions/browser/api/web_request/web_request_api.cc
M net/quic/chromium/quic_stream_factory.cc
M third_party/WebKit/Source/core/dom/Document.cpp
M third_party/WebKit/Source/core/editing/EditingStyle.cpp
M third_party/WebKit/Source/core/page/PageWidgetDelegate.cpp
M third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
M third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
M ui/compositor/layer_animation_element.cc
M ui/views/widget/widget.cc
14 files changed, 37 insertions(+), 31 deletions(-)

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 15, 2018, 1:17:47 AM3/15/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Buck Krasic, Jochen Eisinger, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Fixed a typo

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Jochen Eisinger (Gerrit)

unread,

Mar 15, 2018, 3:32:54 PM3/15/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, jrn, Buck Krasic, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

which files do you want me to look at?

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Buck Krasic (Gerrit)

unread,

Mar 15, 2018, 3:43:38 PM3/15/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Jochen Eisinger, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File net/quic/chromium/quic_stream_factory.cc:
- Patch Set #7, Line 618: }
  Minor nit, prefer to change
  if (session_) {
  }
  to
```
if (!session_)
   return OK;
```
  to avoid indentation.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 15, 2018, 3:48:36 PM3/15/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Buck Krasic, Jochen Eisinger, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 7:

which files do you want me to look at?

chrome/* please. And all other if possible - changes are trivial and tiny. Thanks in advance.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 15, 2018, 3:57:28 PM3/15/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Buck Krasic, Jochen Eisinger, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 7:

Patch Set 7:
which files do you want me to look at?
chrome/* please. And all other if possible - changes are trivial and tiny. Thanks in advance.

Sorry. I meant third_party/*, not chrome/*

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 15, 2018, 4:03:10 PM3/15/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Buck Krasic, Jochen Eisinger, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File net/quic/chromium/quic_stream_factory.cc:

- Patch Set #7, Line 618:

- Minor nit, prefer to change […]
  Done

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Buck Krasic (Gerrit)

unread,

Mar 15, 2018, 4:41:14 PM3/15/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Jochen Eisinger, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 8:Code-Review +1

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 16, 2018, 1:37:28 AM3/16/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Buck Krasic, Jochen Eisinger, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 8:Commit-Queue +2

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 16, 2018, 1:37:36 AM3/16/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Buck Krasic, Jochen Eisinger, jrn, Alexei Svitkine, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

only full committers or CL owner with tryjob access are allowed to trigger CQ

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Jochen Eisinger (Gerrit)

unread,

Mar 16, 2018, 8:45:20 AM3/16/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Buck Krasic, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

2 comments:

File chrome/browser/search/search.cc:
- Patch Set #8, Line 382: DCHECK(profile);
  why is that safe? why not if (browser_context) return false at the beginning of this method?
File chrome/browser/ui/browser_commands.cc:
- Patch Set #8, Line 876: DCHECK(current_tab);
  same here, why not early return?

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 16, 2018, 8:51:33 AM3/16/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

2 comments:

File chrome/browser/search/search.cc:

- Patch Set #8, Line 382: DCHECK(profile);
  why is that safe? why not if (browser_context) return false at the beginning of this method?

- Sounds reasonable. thanks!
File chrome/browser/ui/browser_commands.cc:

- Patch Set #8, Line 876: DCHECK(current_tab);
  same here, why not early return?

- IMO current_tab should not be NULL in commands handlers.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 16, 2018, 9:13:59 AM3/16/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Commit Bot, Peter Kasting, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File chrome/browser/ui/browser_commands.cc:

- Patch Set #8, Line 876: DCHECK(current_tab);

- IMO current_tab should not be NULL in commands handlers.
  Done

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Peter Kasting (Gerrit)

unread,

Mar 16, 2018, 10:56:32 AM3/16/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

2 comments:

File chrome/browser/search/search.cc:

- Patch Set #8, Line 382: DCHECK(profile);

- Sounds reasonable. […]
  I asked for there to be no null check in this function. I think the DCHECK is correct.
File chrome/browser/ui/browser_commands.cc:

- Patch Set #8, Line 876: DCHECK(current_tab);

- Done
  Similarly, I asked for this.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 17, 2018, 4:57:11 AM3/17/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

3 comments:

File chrome/browser/search/search.cc:
- Patch Set #1, Line 382: DCHECK(profile);
  I think |profile| cannot be null here, so I'd remove this check.
  Done
File chrome/browser/search/search.cc:

- Patch Set #8, Line 382: DCHECK(profile);

- I asked for there to be no null check in this function. I think the DCHECK is correct.
  Done
File chrome/browser/ui/browser_commands.cc:

- Patch Set #8, Line 876: DCHECK(current_tab);

- Similarly, I asked for this.
  Done

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 19, 2018, 12:15:57 PM3/19/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 8:Commit-Queue +2

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 19, 2018, 12:16:10 PM3/19/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

only full committers or CL owner with tryjob access are allowed to trigger CQ

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Peter Kasting (Gerrit)

unread,

Mar 19, 2018, 2:22:54 PM3/19/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 9:Commit-Queue +2

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 19, 2018, 2:23:00 PM3/19/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

CQ is trying the patch.

Note: The patchset sent to CQ was uploaded after this CL was approved.

"Rebase" https://chromium-review.googlesource.com/c/955852/9

Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/955852/9

Bot data: {"action": "start", "triggered_at": "2018-03-19T18:22:52.0Z", "cq_cfg_revision": "b6c5f044c073ae207081077d3fd1ff808549a7e0", "revision": "7da43d56804ff1f4b83a9f8dbdad29d23441ba4e"}

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Commit Bot (Gerrit)

unread,

Mar 19, 2018, 3:03:02 PM3/19/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Jochen Eisinger, Buck Krasic, jrn, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Try jobs failed on following builders:

  chromium_presubmit on luci.chromium.try (JOB_FAILED, https://ci.chromium.org/p/chromium/builders/luci.chromium.try/chromium_presubmit/61271)

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 19, 2018, 3:12:08 PM3/19/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Jochen Eisinger, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

6 comments:

File base/metrics/field_trial.cc:
- Patch Set #1, Line 1447: return;
  Nit: Simpler: […]
  Done
- Patch Set #1, Line 1454: } else {
  Nit: While here: change the first arg of this to |allocator| (since that's what it is)
  Done
File chrome/browser/search/search.cc:
- Patch Set #1, Line 382: DCHECK(profile);
  Done
  Done
File chrome/browser/ui/browser_commands.cc:
- Patch Set #1, Line 876: DCHECK(current_tab);
  Various other commands in this file assume an active web contents. […]
  Done
File content/browser/renderer_host/render_widget_host_impl.cc:
- Patch Set #1, Line 1712: RenderViewHostDelegateView* view = delegate_->GetDelegateView();
  Functions like OnStartDragging() assume a delegate, so I think it's OK to remove this check.
  Done
File third_party/WebKit/Source/core/dom/Document.cpp:
- Patch Set #1, Line 2332: return;
  Nit: Change to: […]
  Done

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 19, 2018, 3:18:23 PM3/19/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Toni Barzic, Mathieu Perreault, Dmitry Gozman, Chris Harrelson, danakj, Trent Apted, Peter Kasting, Jochen Eisinger, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Added owners for:

    Missing LGTM from an OWNER for these files:
        chrome/browser/extensions/api/tabs/tabs_api.cc
        chrome/browser/search/search.cc
        extensions/browser/api/web_request/web_request_api.cc
        third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
        third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
        ui/compositor/layer_animation_element.cc
        ui/views/widget/widget.cc

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Chris Harrelson (Gerrit)

unread,

Mar 19, 2018, 3:57:55 PM3/19/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Toni Barzic, Mathieu Perreault, Dmitry Gozman, danakj, Trent Apted, Peter Kasting, Jochen Eisinger, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

3 comments:

File third_party/WebKit/Source/core/dom/Document.cpp:
- Patch Set #9, Line 2344:
  Presumably this code was there to work around a call stack that we couldn't reproduce.
  Is there any reason to suspect that this is no longer the case?
File third_party/WebKit/Source/core/editing/EditingStyle.cpp:
- Patch Set #9, Line 1490: if (style_from_matched_rules &&
  It could be that style_from_matched_rules is guaranteed non-null at this point.
File third_party/WebKit/Source/core/page/PageWidgetDelegate.cpp:
- Patch Set #9, Line 140: if (root && event.GetModifiers() & WebInputEvent::kIsTouchAccessibility &&
  Ditto. I'm not in favor of adding more null checks just because.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Jochen Eisinger (Gerrit)

unread,

Mar 19, 2018, 4:30:16 PM3/19/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Chris Harrelson, Toni Barzic, Mathieu Perreault, Dmitry Gozman, danakj, Trent Apted, Peter Kasting, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

since Peter already reviewed the files in //chrome you asked me to look at, I guess you don't need me?

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Trent Apted (Gerrit)

unread,

Mar 19, 2018, 6:40:46 PM3/19/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Trent Apted, Jochen Eisinger, Chris Harrelson, Toni Barzic, Mathieu Perreault, Dmitry Gozman, danakj, Peter Kasting, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

widget.cc lgtm - there's some repeated logic, but I don't see a neat way to combine it :/

Patch set 9:Code-Review +1

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 20, 2018, 12:24:43 AM3/20/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Trent Apted, Jochen Eisinger, Chris Harrelson, Toni Barzic, Mathieu Perreault, Dmitry Gozman, danakj, Peter Kasting, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

3 comments:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #9, Line 2344:

- Presumably this code was there to work around a call stack that we couldn't reproduce. […]
  Peter Kasting suggested to change "if" to DCHECK. Do you think it worth to fall back to "if" instead?
File third_party/WebKit/Source/core/editing/EditingStyle.cpp:

- Patch Set #9, Line 1490: if (style_from_matched_rules &&
  It could be that style_from_matched_rules is guaranteed non-null at this point.

But 30 lines above there's a check

    if (style_from_matched_rules && !style_from_matched_rules->IsEmpty()) {

File third_party/WebKit/Source/core/page/PageWidgetDelegate.cpp:

- Patch Set #9, Line 140: if (root && event.GetModifiers() & WebInputEvent::kIsTouchAccessibility &&
  Ditto. I'm not in favor of adding more null checks just because.

- But why? root can be null here - see
  127 if (root) {
  128 Document* document = root->GetDocument();
  129 DCHECK(document);
  and later
  168 case WebInputEvent::kMouseMove:
  169 if (!root || !root->View())
  170 return WebInputEventResult::kHandledSuppressed;

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 20, 2018, 12:39:39 AM3/20/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Trent Apted, Jochen Eisinger, Chris Harrelson, Toni Barzic, Mathieu Perreault, Dmitry Gozman, danakj, Peter Kasting, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #9, Line 2344:

Peter Kasting suggested to change "if" to DCHECK. […]

Well, you are right. DCHECK make test fail:

[ RUN      ] WebViewTests/WebViewTest.SelectShowHide/1
...
[5044:2616:0319/115747.993:FATAL:Document.cpp(2343)] Check failed: frame_view.
Backtrace:
	base::debug::StackTrace::StackTrace [0x03AF3560+32]
	base::debug::StackTrace::StackTrace [0x03ABAA9D+13]
	logging::LogMessage::~LogMessage [0x03A61653+83]
	blink::Document::UpdateStyleAndLayout [0x05FABACE+164]
	blink::Document::UpdateStyleAndLayoutIgnorePendingStylesheets [0x05FABA06+22]

Peter Kasting (Gerrit)

unread,

Mar 20, 2018, 2:03:05 AM3/20/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Trent Apted, Jochen Eisinger, Chris Harrelson, Toni Barzic, Mathieu Perreault, Dmitry Gozman, danakj, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #9, Line 2344:

Well, you are right. DCHECK make test fail: […]

I was attempting a non-functional change and mis-rewrote the code. The proposal should presumably have been:

  // |frame_view| can be null when <describe how>.
  DCHECK(!frame_view || !frame_view->IsInPerformLayout())
      << "View layout should not be re-entrant";

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 20, 2018, 3:03:49 AM3/20/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Trent Apted, Jochen Eisinger, Chris Harrelson, Toni Barzic, Mathieu Perreault, Dmitry Gozman, danakj, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #9, Line 2344:

- I was attempting a non-functional change and mis-rewrote the code. […]
  In your code if |frame_view| is nullptr, then DCHECK is satisfied while "if (frame_view->NeedsLayout())" acceesses via nullptr.

danakj (Gerrit)

unread,

Mar 20, 2018, 2:23:56 PM3/20/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Trent Apted, Jochen Eisinger, Chris Harrelson, Toni Barzic, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Please explain what you'd like reviewers to review when you add them, there's 11 people reviewing this CL right now.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Peter Kasting (Gerrit)

unread,

Mar 20, 2018, 2:44:43 PM3/20/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Toni Barzic, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #9, Line 2344:

- In your code if |frame_view| is nullptr, then DCHECK is satisfied while "if (frame_view->NeedsLayout […]
  That's not different from today. Today when |frame_view| is null, the conditional here won't be taken, so we will neither NOTREACHED nor early-return. Your test stack proves we can get a null frame_view here. Therefore it must be true that when |frame_view| is null, IsActive() is also false, and we return before the frame_view->NeedsLayout() conditional.
  The point of rewriting the existing code here as the DCHECK I did is to bring it into compliance with the style guide, which bans handling DCHECK failure. "if (x) { NOTREACHED(); ...do anything }" is a form of handling assertion failure, so I'm trying to avoid that. The resulting DCHECK is also more immediately obvious (to me) about saying that a null |frame_view| is possible; the comment should be even more so.

Toni Barzic (Gerrit)

unread,

Mar 20, 2018, 3:02:06 PM3/20/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Devlin, Peter Kasting, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

+rdevlin.cronin, since this impacts core extensions APIs (rather than platform apps APIs)

Also, splitting this into smaller cls would have been a good idea.

1 comment:

File extensions/browser/api/web_request/web_request_api.cc:
- Patch Set #11, Line 2097: extension
  I think !web_view_instance_id might imply that |extension| is not null, so DCHECK(extension) would be a better option here.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Devlin (Gerrit)

unread,

Mar 20, 2018, 3:15:41 PM3/20/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Devlin, Toni Barzic, Peter Kasting, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

2 comments:

File chrome/browser/extensions/api/tabs/tabs_api.cc:
- Patch Set #11, Line 628: bool set_self_as_opener = create_data &&
  |urls| is populated only if |create_data| is non-null, so this is unnecessary.
File extensions/browser/api/web_request/web_request_api.cc:

- Patch Set #11, Line 2097: extension

- I think !web_view_instance_id might imply that |extension| is not null, so DCHECK(extension) would b […]
  agreed, please make this a DCHECK()

Alexei Svitkine

unread,

Mar 20, 2018, 3:19:25 PM3/20/18

to change...@chromium-review.googlesource.com, Zinovy Nis, asvitkine+watch, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-reviews, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Devlin, Toni Barzic, Peter Kasting, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

I'm confused how you're going about this change. It seems you're continually adding more files and more reviewers after receiving L-G-T-M's.

That doesn't seem to be the right way to go about this. I think once you have a CL fully reviewed and approved, you should land it and if you want to make further changes, do so in separate CLs.

Alexei Svitkine (Gerrit)

unread,

Mar 20, 2018, 3:19:35 PM3/20/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Devlin, Toni Barzic, Peter Kasting, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

I'm confused how you're going about this change. It seems you're continually adding more files and more reviewers after receiving L-G-T-M's.

That doesn't seem to be the right way to go about this. I think once you have a CL fully reviewed and approved, you should land it and if you want to make further changes, do so in separate CLs.

Zinovy Nis (Gerrit)

unread,

Mar 20, 2018, 3:52:32 PM3/20/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Devlin, Toni Barzic, Peter Kasting, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 11:

I'm confused how you're going about this change. It seems you're continually adding more files and more reviewers after receiving L-G-T-M's.
That doesn't seem to be the right way to go about this. I think once you have a CL fully reviewed and approved, you should land it and if you want to make further changes, do so in separate CLs.

Sorry. But I got an error from the building system that some changes are not reviewed by their owners.

Can you please advice me a better way?

Peter Kasting (Gerrit)

unread,

Mar 20, 2018, 3:58:28 PM3/20/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Devlin, Toni Barzic, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 11:

Patch Set 11:
I'm confused how you're going about this change. It seems you're continually adding more files and more reviewers after receiving L-G-T-M's.
That doesn't seem to be the right way to go about this. I think once you have a CL fully reviewed and approved, you should land it and if you want to make further changes, do so in separate CLs.
Sorry. But I got an error from the building system that some changes are not reviewed by their owners.
Can you please advice me a better way?

I think this was mostly about the delta between patch set 2 and 3, where it looked like we went from a state of "mostly reviewed" to "lots more files".

Alexei Svitkine

unread,

Mar 20, 2018, 3:59:18 PM3/20/18

to change...@chromium-review.googlesource.com, asvitkine+watch, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-reviews, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Devlin, Toni Barzic, Peter Kasting, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

It's correct to add reviewers for files you need OWNERS permission for.

But my comment is around the fact that in patchset 1, which Peter and I reviewed, you had 5 files changed. Now you have 14 files changed.

I think you should split off the new files to a new CL (or multiple) and keep this CL limited to the original 5 files.

Alexei Svitkine (Gerrit)

unread,

Mar 20, 2018, 3:59:24 PM3/20/18

to Zinovy Nis, asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

It's correct to add reviewers for files you need OWNERS permission for.

But my comment is around the fact that in patchset 1, which Peter and I reviewed, you had 5 files changed. Now you have 14 files changed.

I think you should split off the new files to a new CL (or multiple) and keep this CL limited to the original 5 files.

Zinovy Nis (Gerrit)

unread,

Mar 21, 2018, 3:28:06 AM3/21/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 11:

It's correct to add reviewers for files you need OWNERS permission for.
But my comment is around the fact that in patchset 1, which Peter and I reviewed, you had 5 files changed. Now you have 14 files changed.
I think you should split off the new files to a new CL (or multiple) and keep this CL limited to the original 5 files.

OK, thanks. Yes, it was my fault to add new files here, but the changes were so trivial that I did want to make a lot of tiny CLs.

Zinovy Nis (Gerrit)

unread,

Mar 21, 2018, 3:28:39 AM3/21/18

to asvitki...@chromium.org, blink-lay...@chromium.org, blink-re...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, donnd...@chromium.org, drott+bl...@chromium.org, eae+bli...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kinuko...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Jochen Eisinger, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch Set 11:

Patch Set 11:
It's correct to add reviewers for files you need OWNERS permission for.
But my comment is around the fact that in patchset 1, which Peter and I reviewed, you had 5 files changed. Now you have 14 files changed.
I think you should split off the new files to a new CL (or multiple) and keep this CL limited to the original 5 files.
OK, thanks. Yes, it was my fault to add new files here, but the changes were so trivial that I did want to make a lot of tiny CLs.

So, I'll fall back this CL to original 5 files.

Zinovy Nis (Gerrit)

unread,

Mar 21, 2018, 3:18:29 PM3/21/18

to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

The CL was reduced to original 5 files.
The remaining files will be uploaded as separate CLs a bit later with all the remarks considered.

Thanks you all for the feedback!

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 21, 2018, 3:19:35 PM3/21/18

to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Chris Pickel, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Chris, please review search.cc from this CL. Thanks.

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 21, 2018, 4:00:29 PM3/21/18

to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Chris Pickel, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

1 comment:

File third_party/WebKit/Source/core/dom/Document.cpp:

- Patch Set #9, Line 2344:

- That's not different from today. […]
  Done

Chris Pickel (Gerrit)

unread,

Mar 22, 2018, 5:49:54 AM3/22/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Chris Pickel, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

LGTM

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Chris Pickel (Gerrit)

unread,

Mar 22, 2018, 5:51:08 AM3/22/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Chris Pickel, Chris Pickel, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

(proper LGTM from committer account)

Patch set 12:Code-Review +1

Zinovy Nis (Gerrit)

unread,

Mar 22, 2018, 5:51:47 AM3/22/18

to asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Chris Pickel, Chris Pickel, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 12:Commit-Queue +2

Commit Bot (Gerrit)

unread,

Mar 22, 2018, 5:51:48 AM3/22/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Chris Pickel, Chris Pickel, Peter Kasting, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

only full committers or CL owner with tryjob access are allowed to trigger CQ

Peter Kasting (Gerrit)

unread,

Mar 22, 2018, 12:24:10 PM3/22/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Chris Pickel, Chris Pickel, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Commit Bot, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Patch set 12:Commit-Queue +2

Commit Bot (Gerrit)

unread,

Mar 22, 2018, 3:27:53 PM3/22/18

to Zinovy Nis, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Peter Kasting, Chris Pickel, Chris Pickel, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Commit Bot merged this change.

Approvals:
  Peter Kasting: Looks good to me; Commit
  Trent Apted: Looks good to me
  Alexei Svitkine: Looks good to me
  Buck Krasic: Looks good to me
  Chris Pickel: Looks good to me

[clang-tidy] Fix access via null pointer to objects

Found with clang-tidy [clang-analyzer-core.CallAndMessage].
Fixed few cases with an incorrect access pattern:

if (ptr && ptr->HasSomeFeature())
  return;

ptr->...  // ptr here can be null!

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I004c92a6335c9d9c0c7fa14b7e8e83279cb431e6
Reviewed-on: https://chromium-review.googlesource.com/955852
Reviewed-by: Chris Pickel <sfi...@chromium.org>
Reviewed-by: Trent Apted <tap...@chromium.org>
Reviewed-by: Buck Krasic <ckr...@chromium.org>
Reviewed-by: Alexei Svitkine <asvi...@chromium.org>
Reviewed-by: Peter Kasting <pkas...@chromium.org>
Commit-Queue: Peter Kasting <pkas...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545199}
---
M base/metrics/field_trial.cc
M chrome/browser/search/search.cc
M chrome/browser/ui/browser_commands.cc
M content/browser/renderer_host/render_widget_host_impl.cc
M third_party/WebKit/Source/core/dom/Document.cpp
5 files changed, 13 insertions(+), 15 deletions(-)

To view, visit change 955852. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 22, 2018, 3:33:09 PM3/22/18

to Commit Bot, asvitki...@chromium.org, blink-re...@chromium.org, blink-...@chromium.org, donnd...@chromium.org, eae+bli...@chromium.org, jered...@chromium.org, jfweit...@chromium.org, kmadhus...@chromium.org, melevi...@chromium.org, samart...@chromium.org, skanuj...@chromium.org, vmpstr...@chromium.org, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, cbentze...@chromium.org, chromium-a...@chromium.org, danakj...@chromium.org, drott+bl...@chromium.org, extension...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, jbauma...@chromium.org, kinuko...@chromium.org, net-r...@chromium.org, pdr+graphi...@chromium.org, piman...@chromium.org, Peter Kasting, Chris Pickel, Chris Pickel, Devlin, Toni Barzic, danakj, Trent Apted, Chris Harrelson, Mathieu Perreault, Dmitry Gozman, Buck Krasic, Jonathan Nieder, Alexei Svitkine, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Kalyan Kondapally, Luna Lu, Stephen Chenney, Thiago Farina, Ian Vollick, chromium...@chromium.org, David Black, John Abd-El-Malek, Rob Buis

Thank you for your assistance!

Zinovy Nis (Gerrit)

unread,

Mar 24, 2018, 9:06:41 AM3/24/18

to blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Stephen White, chromium...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Justin Novosad, Luna Lu, Rob Buis, Stephen Chenney

This change is ready for review.

To view, visit change 979438. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 24, 2018, 9:07:46 AM3/24/18

to Justin Novosad, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Stephen White

Zinovy Nis would like Justin Novosad to review this change.

[clang-tidy] Fix access via null pointer to objects

Found with clang-tidy [clang-analyzer-core.CallAndMessage].

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Change-Id: I90792d9d1d39bd5d029dfb50df34d7af45d317e8
---
M third_party/WebKit/Source/core/editing/EditingStyle.cpp
M third_party/WebKit/Source/core/page/PageWidgetDelegate.cpp
M third_party/WebKit/Source/platform/feature_policy/FeaturePolicy.cpp
M third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp
4 files changed, 6 insertions(+), 4 deletions(-)

To view, visit change 979438. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 26, 2018, 6:22:10 AM3/26/18

to blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Commit Bot, Justin Novosad, Stephen White, chromium...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Luna Lu, Rob Buis, Stephen Chenney

This CL is the next step after https://chromium-review.googlesource.com/c/chromium/src/+/955852

To view, visit change 979438. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Mar 27, 2018, 3:28:13 AM3/27/18

to blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Commit Bot, Justin Novosad, Stephen White, chromium...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Luna Lu, Rob Buis, Stephen Chenney

Gentle ping)

To view, visit change 979438. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Apr 1, 2018, 8:58:30 AM4/1/18

to blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Commit Bot, Justin Novosad, Stephen White, chromium...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Luna Lu, Rob Buis, Stephen Chenney

Gentle ping)

To view, visit change 979438. To unsubscribe, or for help writing mail filters, visit settings.

Nico Weber (Gerrit)

unread,

Apr 2, 2018, 1:01:17 PM4/2/18

to Zinovy Nis, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Commit Bot, Justin Novosad, Stephen White, chromium...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Luna Lu, Rob Buis, Stephen Chenney

I'd actually change this the other way round. Since we have the unprotected calls without known problems, I'd remove the null checks further up instead.

To view, visit change 979438. To unsubscribe, or for help writing mail filters, visit settings.

Zinovy Nis (Gerrit)

unread,

Apr 2, 2018, 2:53:52 PM4/2/18

to blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Nico Weber, Commit Bot, Justin Novosad, Stephen White, chromium...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Luna Lu, Rob Buis, Stephen Chenney

Patch Set 6:

I'd actually change this the other way round. Since we have the unprotected calls without known problems, I'd remove the null checks further up instead.

Maybe replace them with DCHECKs?

To view, visit change 979438. To unsubscribe, or for help writing mail filters, visit settings.

Nico Weber

unread,

Apr 2, 2018, 2:56:48 PM4/2/18

to change...@chromium-review.googlesource.com, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-reviews, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Commit Bot, Justin Novosad, Stephen White, chromium...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Luna Lu, Rob Buis, Stephen Chenney

If you want – but an unqualified method call on a pointer is almost the same as a DCHECK even in release builds (exception: methods that could be marked static but aren't), so personally I would just delete the check.

Nico Weber (Gerrit)

unread,

Apr 2, 2018, 2:56:58 PM4/2/18

to Zinovy Nis, blink-lay...@chromium.org, blink-reviews-p...@chromium.org, blink-rev...@chromium.org, blink-...@chromium.org, drott+bl...@chromium.org, fmalit...@chromium.org, iclella...@chromium.org, kinuko...@chromium.org, pdr+graphi...@chromium.org, Commit Bot, Justin Novosad, Stephen White, chromium...@chromium.org, Dongseong Hwang, Dirk Schulze, Joseph Medley, Luna Lu, Rob Buis, Stephen Chenney

If you want – but an unqualified method call on a pointer is almost the same as a DCHECK even in release builds (exception: methods that could be marked static but aren't), so personally I would just delete the check.

To view, visit change 979438. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

0 new messages

Search

Clear search

Close search

Google apps

Main menu