Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a quota of 5MB per origin for localstorage and sessionstorage #7835

Merged
merged 1 commit into from Oct 9, 2015

Conversation

@iawaknahc
Copy link
Contributor

iawaknahc commented Oct 2, 2015

PR for #6739

Review on Reviewable

@highfive
Copy link

highfive commented Oct 2, 2015

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Ms2ger (or someone else) soon.

@iawaknahc
Copy link
Contributor Author

iawaknahc commented Oct 7, 2015

r? @Ms2ger

@jdm jdm assigned jdm and unassigned Ms2ger Oct 7, 2015
@jdm
Copy link
Member

jdm commented Oct 8, 2015

This looks really good! Just a couple subtleties about counting bytes and some changes for readability before we can merge!
-S-awaiting-review +S-needs-code-changes


Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.


components/net/storage_task.rs, line 43 [r1] (raw file):
I'm inclined to make this a global const value rather than a member.


components/net/storage_task.rs, line 112 [r1] (raw file):
We should update this description with the new behaviour.


components/net/storage_task.rs, line 128 [r1] (raw file):
This doesn't seem necessary.


components/net/storage_task.rs, line 137 [r1] (raw file):
Can we use this instead?

let mut new_total_size = current_total_size + value.len();
if let Some(old_value) = entry.get(&name) {
     new_total_size -= old_value.len();
} else {
    new_total_size += name.len();
};

Also, we should be using .as_bytes().len() instead of .len() for all of these calculations, to account for multibyte characters.


components/net/storage_task.rs, line 147 [r1] (raw file):
We don't need to repeat the total calculations again, do we? Can't we use the value we calculated above?


components/net/storage_task.rs, line 182 [r1] (raw file):
Same comment about byte length.


components/net_traits/storage_task.rs, line 28 [r1] (raw file):
I'm inclined to make this a Result<(bool, Option<DOMString>), ()> instead.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Oct 8, 2015

Great! Go ahead and squash these commits together, please!
-S-awaiting-review +S-needs-squash


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


Comments from the review on Reviewable.io

@jdm
Copy link
Member

jdm commented Oct 8, 2015

Just need to rebase to resolve the merge conflicts now :)

@jdm
Copy link
Member

jdm commented Oct 8, 2015

/build/components/net/storage_task.rs:121:54: 121:60 error: no method named `keys` found for type `&(usize, collections::btree::map::BTreeMap<collections::string::String, collections::string::String>)` in the current scope

/build/components/net/storage_task.rs:121                        .map_or(vec![], |entry| entry.keys().cloned().collect());

                                                                                               ^~~~~~
@iawaknahc
Copy link
Contributor Author

iawaknahc commented Oct 8, 2015

Sorry about that.

@jdm
Copy link
Member

jdm commented Oct 8, 2015

No problem! It's what travis is there to protect against :)

@jdm
Copy link
Member

jdm commented Oct 8, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2015

📌 Commit fb45b0e has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2015

Testing commit fb45b0e with merge a562f6e...

bors-servo pushed a commit that referenced this pull request Oct 8, 2015
Implement a quota of 5MB per origin for localstorage and sessionstorage

PR for #6739

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7835)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2015

💔 Test failed - linux-dev

@jdm
Copy link
Member

jdm commented Oct 8, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2015

Previous build results for android, gonk, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt are reusable. Rebuilding only linux-dev...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2015

💔 Test failed - linux-dev

@jdm
Copy link
Member

jdm commented Oct 9, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2015

Testing commit fb45b0e with merge da27d07...

bors-servo pushed a commit that referenced this pull request Oct 9, 2015
Implement a quota of 5MB per origin for localstorage and sessionstorage

PR for #6739

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7835)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2015

💔 Test failed - mac-rel-wpt

@jdm
Copy link
Member

jdm commented Oct 9, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2015

Testing commit fb45b0e with merge 06e0447...

bors-servo pushed a commit that referenced this pull request Oct 9, 2015
Implement a quota of 5MB per origin for localstorage and sessionstorage

PR for #6739

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7835)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 9, 2015

@bors-servo bors-servo merged commit fb45b0e into servo:master Oct 9, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@iawaknahc iawaknahc deleted the iawaknahc:webstorage-quota branch Oct 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.