libservo: Expand the UserContentManager API.#42288
Conversation
7ae9516 to
c7b8b4c
Compare
mrobinson
left a comment
There was a problem hiding this comment.
Looks good with a couple small things:
| static USER_STYLE_SHEET_ID: AtomicU32 = AtomicU32::new(1); | ||
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, MallocSizeOf, PartialEq, Serialize)] | ||
| pub struct UserStyleSheetId(u32); |
There was a problem hiding this comment.
I think make this private now as it's no longer necessary to expose it.
There was a problem hiding this comment.
This type is referenced in the UserContentManagerAction enum that is used in the embedder->constellation message, so this needs to be public.
| /// Returns the `UserStyleSheetId` of this `UserStyleSheet`. This can be used to remove the | ||
| /// user stylesheet from the `UserContentManager`. | ||
| pub fn id(&self) -> UserStyleSheetId { | ||
| self.id | ||
| } |
There was a problem hiding this comment.
I would remove this as the embedder doesn't need to know about the id (at least yet).
There was a problem hiding this comment.
True, but it is currently used by constellation to filter out the removed UserScripts and UserStyleSheets from the Constellation's state when it receives the UserContentManagerAction message due to a mutation.
There was a problem hiding this comment.
Ah, right. Maybe just use #[doc(hidden)] for this then.
| static USER_SCRIPT_ID: AtomicU32 = AtomicU32::new(1); | ||
|
|
||
| #[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, MallocSizeOf, PartialEq, Serialize)] | ||
| pub struct UserScriptId(u32); |
c7b8b4c to
5383c66
Compare
|
Thanks for the review @mrobinson! I've addressed the recent review comments and pushed the changes, but looks like you might need to approve the new changes again, as the PR doesn't seem to get added to the merge queue automatically. |
The patch adds the following functionality to the per-WebView `UserContentManager` API. - Removing a `UserScript` at that was previously added. - Adding a new `UserStyleSheet` representing a user-origin style sheet. allow removing user script - Removing a previously added `UserStyleSheet`. There might be scope for some improvements in the API: - `UserScript` and `UserStyleSheet` have different ways of representing the source location - a `PathBuf` and `Url` respectively. This is due to how those values are used by the underlying evaluation APIs in script and stylo. More investigation is needed here and could be addressed in future patches. Testing: New unit tests are added for the user stylesheet APIs. Existing tests have been updated to test the removal of user scripts.
|
@mukilan Very odd. That wasn't a requirement before. This seems like it's a change to GitHub, a GitHub bug, or a change in our project configuration that I didn't know about. In any case, it's in the MQ now. |
5383c66 to
b13e136
Compare
The patch adds the following functionality to the per-WebView `UserContentManager` API. - Removing a `UserScript` at that was previously added. - Adding a new `UserStyleSheet` representing a user-origin style sheet. allow removing user script - Removing a previously added `UserStyleSheet`. There might be scope for some improvements in the API: - `UserScript` and `UserStyleSheet` have different ways of representing the source location - a `PathBuf` and `Url` respectively. This is due to how those values are used by the underlying evaluation APIs in script and stylo. More investigation is needed here and could be addressed in future patches. Testing: New unit tests are added for the user stylesheet APIs. Exisiting tests have been updated to test the removal of user scripts. Signed-off-by: Mukilan Thiyagarajan <[email protected]> review comments
b13e136 to
b4f6e09
Compare
The patch adds the following functionality to the per-WebView
UserContentManagerAPI.UserScriptat that was previously added.UserStyleSheetrepresenting a user-origin style sheet. allow removing user scriptUserStyleSheet.There might be scope for some improvements in the API:
UserScriptandUserStyleSheethave different ways of representing the source location - aPathBufandUrlrespectively. This is due to how those values are used by the underlying evaluation APIs in script and stylo. More investigation is needed here and could be addressed in future patches.Testing: New unit tests are added for the user stylesheet APIs. Existing tests have been updated to test the removal of user scripts.