When call getAttribute, HTMLIFrame.sandbox has a bug which don't return changed value.

When call getAttribute, HTMLIFrame.sandbox has a bug which don't return changed value.

This CL is fixing a bug.
If call getAttribute("sandbox"), can not return changed value.
BUG=471688
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192797

[hyunjunekim2, 2015-03-28 03:51:32]

hyunjune.kim@samsung.com changed reviewers:
+ fs@opera.com, mkwst@chromium.org

[hyunjunekim2, 2015-03-28 03:53:44]

Hi, I'm hyunjunekim (hyunjune.kim@samsung.com, hykim0777@gamil.com). I fixed up
a bug in IFrame.sandbox.
At night, I am going to make test-cases about this one.
Thank you. :)

[hyunjunekim2, 2015-03-28 13:10:32]

On 2015/03/28 03:53:44, hyunjunekim2 wrote:
> Hi, I'm hyunjunekim (mailto:hyunjune.kim@samsung.com,
mailto:hykim0777@gamil.com). I fixed up
> a bug in IFrame.sandbox.
> At night, I am going to make test-cases about this one.
> Thank you. :)

Mike West, I uploaded test-case.
Could you review this one? Thank you. :)

[Mike West, 2015-03-30 05:01:57]

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
File Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
Source/core/html/HTMLIFrameElement.cpp:168: if (tokenList == m_sandbox) {
When wouldn't this be true? Can you either `ASSERT_EQ(tokenList, m_sandbox)`, or
just drop the parameter entirely?

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
Source/core/html/HTMLIFrameElement.cpp:173:
setSynchronizedLazyAttribute(sandboxAttr, m_sandbox->value());
This line is the only actual change in the patch, right? The rest seems
irrelevant. Why add the complexity?

[hyunjunekim2, 2015-03-30 07:02:51]

Mike West, Could you check my reply?

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
File Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
Source/core/html/HTMLIFrameElement.cpp:168: if (tokenList == m_sandbox) {
On 2015/03/30 05:01:57, Mike West wrote:
> When wouldn't this be true? Can you either `ASSERT_EQ(tokenList, m_sandbox)`,
or
> just drop the parameter entirely?
If HTMLIFrameElement has two DOMSettableTokenList, we should distinguish whether
is sandbox or others about DOMSettableTokenList.

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
Source/core/html/HTMLIFrameElement.cpp:173:
setSynchronizedLazyAttribute(sandboxAttr, m_sandbox->value());
On 2015/03/30 05:01:57, Mike West wrote:
> This line is the only actual change in the patch, right? The rest seems
> irrelevant. Why add the complexity?

I have a reason adding this one. Because it add this line to fix a bug.
<iframe id="if17" sandbox ="hi"/>
<script>
  if17.sandbox = "allow-script";
  if17.getAttribute("sandbox");
</script>
If you run this script, can't return "allow-script", maybe you should see "hi".
So, I think that the blink need this
line(setSynchronizedLazyAttribute(sandboxAttr, m_sandbox->value());) to update
element's attribute value.

[Mike West, 2015-03-30 09:01:33]

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
File Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
Source/core/html/HTMLIFrameElement.cpp:168: if (tokenList == m_sandbox) {
On 2015/03/30 at 07:02:51, hyunjunekim2 wrote:
> On 2015/03/30 05:01:57, Mike West wrote:
> > When wouldn't this be true? Can you either `ASSERT_EQ(tokenList,
m_sandbox)`, or
> > just drop the parameter entirely?
> If HTMLIFrameElement has two DOMSettableTokenList, we should distinguish
whether is sandbox or others about DOMSettableTokenList.

But HTMLIFrameElement doesn't have two DOMSettableTokenLists, does it? If we
need this complexity in the future, we can add it. For now, it just makes this
code more complicated without adding any functionality or value.

For the moment, I'd prefer that you just make the one-line change you need in
order to fix the bug you've identified.

(And, speaking of which, please file a bug at `https://crbug.com/new`, and add
it to the CL description (`BUG=...`).)

[hyunjunekim2, 2015-03-30 09:16:22]

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
File Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
Source/core/html/HTMLIFrameElement.cpp:168: if (tokenList == m_sandbox) {
On 2015/03/30 09:01:33, Mike West wrote:
> On 2015/03/30 at 07:02:51, hyunjunekim2 wrote:
> > On 2015/03/30 05:01:57, Mike West wrote:
> > > When wouldn't this be true? Can you either `ASSERT_EQ(tokenList,
> m_sandbox)`, or
> > > just drop the parameter entirely?
> > If HTMLIFrameElement has two DOMSettableTokenList, we should distinguish
> whether is sandbox or others about DOMSettableTokenList.
> 
> But HTMLIFrameElement doesn't have two DOMSettableTokenLists, does it? If we
> need this complexity in the future, we can add it. For now, it just makes this
> code more complicated without adding any functionality or value.
> 
> For the moment, I'd prefer that you just make the one-line change you need in
> order to fix the bug you've identified.
> 
> (And, speaking of which, please file a bug at `https://crbug.com/new`, and add
> it to the CL description (`BUG=...`).)

Ok, I'm wrong, It's also my mistake. After file a bug, I will request review.

[hyunjunekim2, 2015-03-30 10:00:49]

On 2015/03/30 09:16:22, hyunjunekim2 wrote:
>
https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
> File Source/core/html/HTMLIFrameElement.cpp (right):
> 
>
https://codereview.chromium.org/1040943002/diff/40001/Source/core/html/HTMLIF...
> Source/core/html/HTMLIFrameElement.cpp:168: if (tokenList == m_sandbox) {
> On 2015/03/30 09:01:33, Mike West wrote:
> > On 2015/03/30 at 07:02:51, hyunjunekim2 wrote:
> > > On 2015/03/30 05:01:57, Mike West wrote:
> > > > When wouldn't this be true? Can you either `ASSERT_EQ(tokenList,
> > m_sandbox)`, or
> > > > just drop the parameter entirely?
> > > If HTMLIFrameElement has two DOMSettableTokenList, we should distinguish
> > whether is sandbox or others about DOMSettableTokenList.
> > 
> > But HTMLIFrameElement doesn't have two DOMSettableTokenLists, does it? If we
> > need this complexity in the future, we can add it. For now, it just makes
this
> > code more complicated without adding any functionality or value.
> > 
> > For the moment, I'd prefer that you just make the one-line change you need
in
> > order to fix the bug you've identified.
> > 
> > (And, speaking of which, please file a bug at `https://crbug.com/new`, and
add
> > it to the CL description (`BUG=...`).)
> 
> Ok, I'm wrong, It's also my mistake. After file a bug, I will request review.

Mike West,
Could you give me feedback? if it's available.

[Mike West, 2015-03-30 10:20:52]

LGTM.

[hyunjunekim2, 2015-03-31 00:01:33]

On 2015/03/30 10:20:52, Mike West wrote:
> LGTM.
Mike West,
Could you run try-bot and commit bot? Thank you.

[hyunjunekim2, 2015-03-31 04:30:10]

The CQ bit was checked by hyunjune.kim@samsung.com

[commit-bot: I haz the power, 2015-03-31 04:30:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1040943002/60001

[hyunjunekim2, 2015-03-31 04:31:03]

I request marge to commit-bot.

[commit-bot: I haz the power, 2015-03-31 04:34:20]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192797