Bug 101640 - Have NetworkProcess do the actual loading of subresources
Summary: Have NetworkProcess do the actual loading of subresources
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 98537 210779
  Show dependency treegraph
 
Reported: 2012-11-08 13:43 PST by Brady Eidson
Modified: 2020-04-20 20:16 PDT (History)
8 users (show)

See Also:


Attachments
Patch v1 - Do basic subresource loading (58.23 KB, patch)
2012-11-08 14:00 PST, Brady Eidson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2012-11-08 13:43:47 PST
Have NetworkProcess do the actual loading of subresources

For this first run it just buffers up the entire resource until it's done loading and hands it to the WebProcess in one chunk.

We'll have to do many things such as notify the WebProcess progressively, handle authentication, etc etc.  Those are separable tasks on top of this first patch.
Comment 1 Brady Eidson 2012-11-08 14:00:58 PST
Created attachment 173114 [details]
Patch v1 - Do basic subresource loading

Laced with FIXMEs and obviously with much of the task ahead, this patch at least gets actual networking happening in the NetworkProcess.
Comment 2 WebKit Review Bot 2012-11-08 14:04:20 PST
Attachment 173114 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/NetworkProcess/NetworkRequest.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/Shared/ShareableResource.cpp:34:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alexey Proskuryakov 2012-11-08 14:19:19 PST
Comment on attachment 173114 [details]
Patch v1 - Do basic subresource loading

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

> Source/WebKit2/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

OOPS.

> Source/WebKit2/ChangeLog:65
> +        Add an implementation of WebCore::ResourceBuffer that wraps a ShareableResource instead of a SharedBuffer:

Hmm, it's very much not cool that we still have an m_sharedBuffer data member.

> Source/WebKit2/Shared/ShareableResource.cpp:34
> +ShareableResource::Handle::Handle()
> +{}

Braces go on separate lines.

> Source/WebKit2/Shared/ShareableResource.cpp:61
> +    // Create the shared memory.

Does this comment help? I'm not sure if it's accurate.

> Source/WebKit2/Shared/ShareableResource.cpp:75
> +    ASSERT(m_sharedMemory);
> +    ASSERT(m_offset + m_size <= m_sharedMemory->size());

Should these checks be made in production builds, too?
Comment 4 Brady Eidson 2012-11-08 14:29:19 PST
(In reply to comment #3)
> (From update of attachment 173114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173114&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).
> 
> OOPS.
> 
> > Source/WebKit2/ChangeLog:65
> > +        Add an implementation of WebCore::ResourceBuffer that wraps a ShareableResource instead of a SharedBuffer:
> 
> Hmm, it's very much not cool that we still have an m_sharedBuffer data member.

Agreed.  Can be unraveled separately.
> 
> > Source/WebKit2/Shared/ShareableResource.cpp:34
> > +ShareableResource::Handle::Handle()
> > +{}
> 
> Braces go on separate lines.

You and Style-bot agree!

> 
> > Source/WebKit2/Shared/ShareableResource.cpp:61
> > +    // Create the shared memory.
> 
> Does this comment help? I'm not sure if it's accurate.

Leftovers.  Removed.

> > Source/WebKit2/Shared/ShareableResource.cpp:75
> > +    ASSERT(m_sharedMemory);
> > +    ASSERT(m_offset + m_size <= m_sharedMemory->size());
> 
> Should these checks be made in production builds, too?

Added a fixe with a reasonable course of action (assume the other process is compromised and kill it)
Comment 5 Brady Eidson 2012-11-08 15:05:46 PST
http://trac.webkit.org/changeset/133957