Bug 84542 - [chromium][workers] setTargetType(ResourceRequest::TargetIsWorker) is repeatedly called in chromium
Summary: [chromium][workers] setTargetType(ResourceRequest::TargetIsWorker) is repeate...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-21 23:47 PDT by Li Yin
Modified: 2012-04-25 07:51 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.66 KB, patch)
2012-04-22 00:09 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (1.73 KB, patch)
2012-04-24 00:28 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (1.82 KB, patch)
2012-04-24 01:13 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (2.14 KB, patch)
2012-04-24 22:59 PDT, Li Yin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2012-04-21 23:47:27 PDT
In chromium platform, the default value of m_targetType is ResourceRequest::TargetIsWorker, it isn't necessary to call
worker->m_scriptLoader->setTargetType(ResourceRequest::TargetIsWorker) again in Worker.cpp.
Comment 1 Li Yin 2012-04-22 00:09:18 PDT
Created attachment 138265 [details]
Patch
Comment 2 Li Yin 2012-04-23 23:00:45 PDT
Would you please have a look?
Comment 3 David Levin 2012-04-23 23:29:45 PDT
Comment on attachment 138265 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138265&action=review

> Source/WebCore/ChangeLog:12
> +        No new tests. The current test has covered it already.

Which test?

> Source/WebCore/workers/Worker.cpp:75
>  #endif

Should we add an assert here to verify the target type?
Comment 4 Li Yin 2012-04-23 23:51:05 PDT
(In reply to comment #3)
> (From update of attachment 138265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138265&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests. The current test has covered it already.
> 
> Which test?
This patch just deleted a duplicated statement. It can't impact any features. So no test case is required.
> 
> > Source/WebCore/workers/Worker.cpp:75
> >  #endif
> 
> Should we add an assert here to verify the target type?
There isn't public function to get the target type in current code, if we need to assert the target type, I must add the new function for chromium platform, it indeed adds the difficulty of code.
And in WorkerScriptLoader Constructor, there is a obvious statement about the target type, and no change about the value. Maybe it isn't necessary to add the assert statement. do you think so?

Thanks for your review.
Comment 5 David Levin 2012-04-24 00:00:33 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 138265 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=138265&action=review
> > 
> > > Source/WebCore/ChangeLog:12
> > > +        No new tests. The current test has covered it already.
> > 
> > Which test?
> This patch just deleted a duplicated statement. It can't impact any features. So no test case is required.

You said in the change log that "The current test has covered it already."

Which test covers this? Which test will fail if this value is incorrect? (It would be nice to make sure something is going to fail if someone changes the default value in chromium.)

Why is this change being done? :)

> > 
> > > Source/WebCore/workers/Worker.cpp:75
> > >  #endif
> > 
> > Should we add an assert here to verify the target type?
> There isn't public function to get the target type in current code, if we need to assert the target type, I must add the new function for chromium platform, it indeed adds the difficulty of code.
> And in WorkerScriptLoader Constructor, there is a obvious statement about the target type, and no change about the value. Maybe it isn't necessary to add the assert statement. do you think so?
> 
> Thanks for your review.
Comment 6 Li Yin 2012-04-24 00:10:59 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 138265 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=138265&action=review
> > > 
> > > > Source/WebCore/ChangeLog:12
> > > > +        No new tests. The current test has covered it already.
> > > 
> > > Which test?
> > This patch just deleted a duplicated statement. It can't impact any features. So no test case is required.
> 
> You said in the change log that "The current test has covered it already."
> 
> Which test covers this? Which test will fail if this value is incorrect? (It would be nice to make sure something is going to fail if someone changes the default value in chromium.)
> 
> Why is this change being done? :)
> 
Okay, I will change the log, thanks for your detailed review.
Comment 7 Li Yin 2012-04-24 00:28:48 PDT
Created attachment 138509 [details]
Patch
Comment 8 Li Yin 2012-04-24 01:12:27 PDT
Add the detailed description in ChangLog, please have a look, thanks.
Comment 9 Li Yin 2012-04-24 01:13:51 PDT
Created attachment 138513 [details]
Patch
Comment 10 David Levin 2012-04-24 08:42:25 PDT
The ChangeLog doesn't answer the question: What test would fail if this value is incorrect? (It would be nice to know that there is one.)
Comment 11 David Levin 2012-04-24 08:43:36 PDT
Comment on attachment 138513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138513&action=review

> Source/WebCore/ChangeLog:13
> +        No new tests because this patch just deletes a repeated code, it can reduce code workload and can't impact any feature.

This may impact features if the target type is set incorrectly and may result in a change in behavior so it would be good to know the test that would expose if there were this problem.
Comment 12 Li Yin 2012-04-24 22:59:11 PDT
Created attachment 138740 [details]
Patch
Comment 13 David Levin 2012-04-24 23:03:41 PDT
Comment on attachment 138740 [details]
Patch

I wonder why http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/workers/WorkerScriptLoader.cpp&type=cs&l=55 isn't changed to be "|| PLATFORM(BLACKBERRY)" and then just get rid of this.

But this is fine as is.  Thanks!
Comment 14 Li Yin 2012-04-24 23:31:28 PDT
(In reply to comment #13)
> (From update of attachment 138740 [details])
> I wonder why http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/workers/WorkerScriptLoader.cpp&type=cs&l=55 isn't changed to be "|| PLATFORM(BLACKBERRY)" and then just get rid of this.
> 
> But this is fine as is.  Thanks!

You should look this linked address. http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/workers/Worker.cpp&type=cs&l=73
Comment 15 David Levin 2012-04-25 07:45:54 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 138740 [details] [details])
> > I wonder why http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/workers/WorkerScriptLoader.cpp&type=cs&l=55 isn't changed to be "|| PLATFORM(BLACKBERRY)" and then just get rid of this.
> > 
> > But this is fine as is.  Thanks!
> 
> You should look this linked address. http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/workers/Worker.cpp&type=cs&l=73

I don't understand what you are saying. The line that you referred to is the code you are changing.

Here's what I was saying. On blackberry, this field is uninitialized because on this line
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/workers/WorkerScriptLoader.cpp&type=cs&l=55

it is only initialized for Chromium.

If we changed that line to also initialize the field for blackberry, then this line
http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/workers/Worker.cpp&type=cs&l=73

would be necessary on blackberry either and it could be removed.
Comment 16 WebKit Review Bot 2012-04-25 07:51:21 PDT
Comment on attachment 138740 [details]
Patch

Clearing flags on attachment: 138740

Committed r115207: <http://trac.webkit.org/changeset/115207>
Comment 17 WebKit Review Bot 2012-04-25 07:51:27 PDT
All reviewed patches have been landed.  Closing bug.