Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646493002:
'*' is not a valid attribute selector (Closed)
Description'*' is not a valid attribute selector
Per the CSS Selectors specification [1], a '*' is not allowed as the
name of an attribute.
[1] https://drafts.csswg.org/selectors-4/#typedef-attribute-selector
BUG=681814
Review-Url: https://codereview.chromium.org/2646493002
Cr-Commit-Position: refs/heads/master@{#445046}
Committed: https://chromium.googlesource.com/chromium/src/+/0b9363c48f13ffd33d88dd9e810697fd86c2b301
Patch Set 1 #Patch Set 2 : Disallow starAtom as attributeName #Patch Set 3 : Adjust serialization test #
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by fs@opera.com 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...
Description was changed from ========== Attribute selectors cannot be 'universal' (*) Per the CSS Selectors specification [1], a '*' is not allowed as the name of an attribute. [1] https://drafts.csswg.org/selectors-4/#typedef-attribute-selector BUG=681814 ========== to ========== '*' is not a valid attribute selector Per the CSS Selectors specification [1], a '*' is not allowed as the name of an attribute. [1] https://drafts.csswg.org/selectors-4/#typedef-attribute-selector BUG=681814 ==========
fs@opera.com changed reviewers: + rune@opera.com, timloh@chromium.org
Noticed while working on the referenced bug. I can't say I find this solution particularly pretty, so suggestions welcome... =)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/18 16:34:13, fs wrote: > Noticed while working on the referenced bug. I can't say I find this solution > particularly pretty, so suggestions welcome... =) Maybe it's easier to just reject if the returned named == starAtom?
On 2017/01/19 00:44:54, Timothy Loh wrote: > On 2017/01/18 16:34:13, fs wrote: > > Noticed while working on the referenced bug. I can't say I find this solution > > particularly pretty, so suggestions welcome... =) > > Maybe it's easier to just reject if the returned named == starAtom? My thought too, but I don't know the parser that well and didn't know if that would cause incorrect error correction.
On 2017/01/19 at 08:17:00, rune wrote: > On 2017/01/19 00:44:54, Timothy Loh wrote: > > On 2017/01/18 16:34:13, fs wrote: > > > Noticed while working on the referenced bug. I can't say I find this solution > > > particularly pretty, so suggestions welcome... =) > > > > Maybe it's easier to just reject if the returned named == starAtom? > > My thought too, but I don't know the parser that well and didn't know if that would cause incorrect error correction. Theoretically we can't distinguish an escaped '*' from a '*' (i.e they will both map/hash to the same StringImpl and hence the starAtom.) If we don't mind that (admittedly extreme corner case), then we could reject based on name == starAtom. I think we may have that issue in other cases already, so it may not be terrible.
On 2017/01/19 14:31:37, fs wrote: > On 2017/01/19 at 08:17:00, rune wrote: > > On 2017/01/19 00:44:54, Timothy Loh wrote: > > > On 2017/01/18 16:34:13, fs wrote: > > > > Noticed while working on the referenced bug. I can't say I find this > solution > > > > particularly pretty, so suggestions welcome... =) > > > > > > Maybe it's easier to just reject if the returned named == starAtom? > > > > My thought too, but I don't know the parser that well and didn't know if that > would cause incorrect error correction. > > Theoretically we can't distinguish an escaped '*' from a '*' (i.e they will both > map/hash to the same StringImpl and hence the starAtom.) If we don't mind that > (admittedly extreme corner case), then we could reject based on name == > starAtom. I think we may have that issue in other cases already, so it may not > be terrible. I've reported https://crbug.com/682747
On 2017/01/19 18:41:25, rune wrote: > On 2017/01/19 14:31:37, fs wrote: > > Theoretically we can't distinguish an escaped '*' from a '*' (i.e they will > both > > map/hash to the same StringImpl and hence the starAtom.) If we don't mind that > > (admittedly extreme corner case), then we could reject based on name == > > starAtom. I think we may have that issue in other cases already, so it may not > > be terrible. > > I've reported https://crbug.com/682747 I'd say it'd be fine to have the same issue for escaped '*' in attribute selectors as we have for type selectors. Fixing one will then fix the other.
The CQ bit was checked by fs@opera.com to run a CQ dry run
On 2017/01/19 at 21:07:27, rune wrote: > On 2017/01/19 18:41:25, rune wrote: > > On 2017/01/19 14:31:37, fs wrote: > > > > Theoretically we can't distinguish an escaped '*' from a '*' (i.e they will > > both > > > map/hash to the same StringImpl and hence the starAtom.) If we don't mind that > > > (admittedly extreme corner case), then we could reject based on name == > > > starAtom. I think we may have that issue in other cases already, so it may not > > > be terrible. > > > > I've reported https://crbug.com/682747 > > I'd say it'd be fine to have the same issue for escaped '*' in attribute selectors as we have for type selectors. Fixing one will then fix the other. Ok, done in PS2.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, but timloh@ should have a say.
On 2017/01/19 22:53:45, rune wrote: > lgtm, but timloh@ should have a say. lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fs@opera.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org, rune@opera.com Link to the patchset: https://codereview.chromium.org/2646493002/#ps40001 (title: "Adjust serialization test")
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": 40001, "attempt_start_ts": 1484918719434070,
"parent_rev": "79fd9eab61da2ac2bb5261b460ed9f5afb6d4228", "commit_rev":
"0b9363c48f13ffd33d88dd9e810697fd86c2b301"}
Message was sent while issue was closed.
Description was changed from ========== '*' is not a valid attribute selector Per the CSS Selectors specification [1], a '*' is not allowed as the name of an attribute. [1] https://drafts.csswg.org/selectors-4/#typedef-attribute-selector BUG=681814 ========== to ========== '*' is not a valid attribute selector Per the CSS Selectors specification [1], a '*' is not allowed as the name of an attribute. [1] https://drafts.csswg.org/selectors-4/#typedef-attribute-selector BUG=681814 Review-Url: https://codereview.chromium.org/2646493002 Cr-Commit-Position: refs/heads/master@{#445046} Committed: https://chromium.googlesource.com/chromium/src/+/0b9363c48f13ffd33d88dd9e8106... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/0b9363c48f13ffd33d88dd9e8106... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
