|
|
DescriptionRemoved CSSPropertyAPIShadow group property stub
The CSSPropertyAPIShadow stub was intended to implement a shared API
between the CSSPropertyTextShadow and CSSPropertyBoxShadow properties.
This stub was added prematurely, and will in fact not be used as
these properties have recently been refactored and require unique
parsing logic, so they will be individually implemented as APIs in
future.
BUG=668012
Review-Url: https://codereview.chromium.org/2877783004
Cr-Commit-Position: refs/heads/master@{#471668}
Committed: https://chromium.googlesource.com/chromium/src/+/6a1960fb48be21c84024bec173b72da40d16289d
Patch Set 1 #
Messages
Total messages: 30 (13 generated)
The CQ bit was checked by bugsnash@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bugsnash@chromium.org changed reviewers: + jiameng@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2017/05/12 06:19:31, Bugs Nash wrote: LGTM
bugsnash@chromium.org changed reviewers: + suzyh@chromium.org
+suzyh for owners
I would have expected one patch to cover both the deletion of the old stub and the creation of new stubs for easier code archaeology, but it doesn't seem an important enough point to block this CL. lgtm
On 2017/05/15 at 02:05:19, suzyh wrote: > I would have expected one patch to cover both the deletion of the old stub and the creation of new stubs for easier code archaeology, but it doesn't seem an important enough point to block this CL. > > lgtm I actually have a local patch that adds these stubs as well as all the other missing stubs (there are 22) but I decided it didn't make any sense to add stubs - code that wasn't going to be used yet. Do you think I should upload that patch?
The CQ bit was checked by bugsnash@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/05/15 at 02:44:56, bugsnash wrote: > On 2017/05/15 at 02:05:19, suzyh wrote: > > I would have expected one patch to cover both the deletion of the old stub and the creation of new stubs for easier code archaeology, but it doesn't seem an important enough point to block this CL. > > > > lgtm > > I actually have a local patch that adds these stubs as well as all the other missing stubs (there are 22) but I decided it didn't make any sense to add stubs - code that wasn't going to be used yet. > Do you think I should upload that patch? Hmm.. in general no I don't think it makes sense to upload unused stubs since it easily becomes this case where, as we get further along, we realise that we don't need them after all. I only suggested it here because the CL description sort of implies that you're replacing one stub with two others. Two options present themselves: - Add stubs for CSSPropertyTextShadow and CSSPropertyBoxShadow in this patch. This only really makes sense if you're about to add content for them, and is only for tracing a clear path for things through the code history. - In the CL description, clarify that this patch is deleting prematurely-added-now-unneeded code, rather than replacing it. All things considered I'd suggest option #2.
On 2017/05/15 at 02:52:20, commit-bot wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Looks like you're going to need core/ OWNERS for the BUILD.gn change, which I am not, sorry!
On 2017/05/15 at 02:54:57, suzyh wrote: > On 2017/05/15 at 02:44:56, bugsnash wrote: > > On 2017/05/15 at 02:05:19, suzyh wrote: > > > I would have expected one patch to cover both the deletion of the old stub and the creation of new stubs for easier code archaeology, but it doesn't seem an important enough point to block this CL. > > > > > > lgtm > > > > I actually have a local patch that adds these stubs as well as all the other missing stubs (there are 22) but I decided it didn't make any sense to add stubs - code that wasn't going to be used yet. > > Do you think I should upload that patch? > > Hmm.. in general no I don't think it makes sense to upload unused stubs since it easily becomes this case where, as we get further along, we realise that we don't need them after all. I only suggested it here because the CL description sort of implies that you're replacing one stub with two others. Two options present themselves: > - Add stubs for CSSPropertyTextShadow and CSSPropertyBoxShadow in this patch. This only really makes sense if you're about to add content for them, and is only for tracing a clear path for things through the code history. > - In the CL description, clarify that this patch is deleting prematurely-added-now-unneeded code, rather than replacing it. > > All things considered I'd suggest option #2. Agreed I like option #2.
On 2017/05/15 at 02:55:56, suzyh wrote: > On 2017/05/15 at 02:52:20, commit-bot wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > Looks like you're going to need core/ OWNERS for the BUILD.gn change, which I am not, sorry! Ah, my bad. Thanks :)
Description was changed from ========== Removed CSSPropertyAPIShadow group property stub The CSSPropertyAPIShadow stub was intended to implement a shared API between the CSSPropertyTextShadow and CSSPropertyBoxShadow properties. These properties have recently been refactored and require unique parsing logic, so they will now be individually implemented as APIs. BUG=668012 ========== to ========== Removed CSSPropertyAPIShadow group property stub The CSSPropertyAPIShadow stub was intended to implement a shared API between the CSSPropertyTextShadow and CSSPropertyBoxShadow properties. This stub was added prematurely, and will in fact not be used as these properties have recently been refactored and require unique parsing logic, so they will be individually implemented as APIs in future. BUG=668012 ==========
bugsnash@chromium.org changed reviewers: + alancutter@chromium.org
On 2017/05/15 at 03:05:02, Bugs Nash wrote: > +alancutter for core owners
lgtm. We should avoid landing unused code in future until we have dependent patches that use it and are ready to land as well.
On 2017/05/15 at 04:29:19, alancutter wrote: > lgtm. We should avoid landing unused code in future until we have dependent patches that use it and are ready to land as well. agreed
The CQ bit was checked by bugsnash@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 1, "attempt_start_ts": 1494822760586100, "parent_rev": "8ace54b1623362bd0720adf22811f237d93344a0", "commit_rev": "6a1960fb48be21c84024bec173b72da40d16289d"}
Message was sent while issue was closed.
Description was changed from ========== Removed CSSPropertyAPIShadow group property stub The CSSPropertyAPIShadow stub was intended to implement a shared API between the CSSPropertyTextShadow and CSSPropertyBoxShadow properties. This stub was added prematurely, and will in fact not be used as these properties have recently been refactored and require unique parsing logic, so they will be individually implemented as APIs in future. BUG=668012 ========== to ========== Removed CSSPropertyAPIShadow group property stub The CSSPropertyAPIShadow stub was intended to implement a shared API between the CSSPropertyTextShadow and CSSPropertyBoxShadow properties. This stub was added prematurely, and will in fact not be used as these properties have recently been refactored and require unique parsing logic, so they will be individually implemented as APIs in future. BUG=668012 Review-Url: https://codereview.chromium.org/2877783004 Cr-Commit-Position: refs/heads/master@{#471668} Committed: https://chromium.googlesource.com/chromium/src/+/6a1960fb48be21c84024bec173b7... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/6a1960fb48be21c84024bec173b7... |