Closed Bug 1301126 Opened 3 years ago Closed 3 years ago

Restrict the scroll grabbing feature only to chrome consumers

Categories

(Core :: Panning and Zooming, defect, P5)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: ehsan, Assigned: kevchan85, Mentored)

References

Details

(Whiteboard: [gfx-noted] [lang=c++] [good first bug])

Attachments

(1 file, 2 obsolete files)

See bug 912666 comment 88.  Certified apps aren't a thing any more.
OS: Unspecified → All
Priority: -- → P5
Hardware: Unspecified → All
Whiteboard: [gfx-noted]
Version: unspecified → 51 Branch
This would make a good mentored bug. A good first bug, even.
Mentor: botond
Whiteboard: [gfx-noted] → [gfx-noted] [lang=c++] [good first bug]
Hello! 

Can I take this as my first bug please? Is there any documentation I should read? The Mozilla code base is so big I don't know where to begin... 

Thank you

Oh... and once I start how do I test my code?
Flags: needinfo?(botond)
Hi Kevin! Thanks for your interest in contributing.

The first step is to build Firefox from source using the instructions here [1], if you haven't done so already.

Once you have a successful build, let me know and I'll tell you where in the code to look to fix this problem.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Flags: needinfo?(botond)
Hello, 

So I have downloaded the and built firefox... I see that Mozilla does not use the standard library. I usually use an IDE for my coding (xcode) is that going to be a problem? I can adapt but I would like to learn how...
Flags: needinfo?(botond)
(In reply to kevin from comment #4)
> I see that Mozilla does not use the standard library. 

We use some parts of the standard library, and not others. The code has a long history, dating back to a time when standard library implementations weren't as high-quality as they are today.

> I usually use an IDE for my coding (xcode) is that going to be a problem? 

Not at all! You can use any editor or IDE for working on the Firefox code; I use Eclipse CDT, but I also know people who use other things, including Xcode.

There may even be a way to generate an Xcode project from the Firefox build system, but I don't know off the top of my head. I'll try to find out and get back to you.


In the meantime, let me point you to the relevant code for this bug.

The "scroll grabbing feature" mentioned in the bug title refers to the "scrollgrab" attribute on an HTMLElement [1]. (The details of what it doesn't aren't important; if you're interested, you can read bug 912666 which introduced it).

Notice that above the attribute there the following annotation:

  [Func="nsGenericHTMLElement::IsScrollGrabAllowed"]

This means the attribute is only exposed if the function nsGenericHTMLElement::IsScrollGrabAllowed returns true.

The implementation of this function can be found here [2]. Currently, it returns true in two cases: 

  1) If the calling code has chrome privileges.
     This is the case for the code that implements the browser's UI, but not for code
     running as part of a webpage.
     This is what IsSystemPrincipal() tests for.

  2) If the calling code is running inside a certified app.
     This refers to a category of Firefox OS [3] apps which receive certain additional
     permissions. (Firefox OS is powered by the same rendering engine as the Firefox
     browser, so they share a lot of the code.)

The objective of this bug is remove case (2), since support for Firefox OS is being discontinued.


I think that should be enough to get you started. If you have any questions, please feel free to ask!

Once you've made the change to the code, please build again to make sure the code still builds, and then post a patch for me to review. You can either post the patch file as an attachment to this bug, or use our new "MozReview" code review system [4] - whichever you prefer.

[1] http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/dom/webidl/HTMLElement.webidl#88
[2] http://searchfox.org/mozilla-central/rev/950e13cca1fda6507dc53c243295044e8eda4493/dom/html/nsGenericHTMLElement.cpp#1677
[3] https://www.mozilla.org/en-US/firefox/os/
[4] http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html
Flags: needinfo?(botond)
Assignee: nobody → kevchan85
(In reply to Botond Ballo [:botond] from comment #5)
> There may even be a way to generate an Xcode project from the Firefox build
> system, but I don't know off the top of my head.

Perhaps Markus knows.
Flags: needinfo?(mstange)
I don't think there is a supported way to do this at the moment. Your best bet is to use XCode purely as an editor, and build from the command line, as described in all of our documentation.
We have bug 1063329 for adding XCode project generation.
Flags: needinfo?(mstange)
So I removed the second part of the or statement from...
  return nsContentUtils::IsSystemPrincipal(prin) ||
    prin->GetAppStatus() == nsIPrincipal::APP_STATUS_CERTIFIED;

I made a patch file... but since I deleted a line it caused a lot of functions to shift up.
Flags: needinfo?(botond)
Attachment #8790117 - Flags: review?(botond)
Comment on attachment 8790117 [details] [diff] [review]
bug1301126_Restrict_Scroll_to_chrome.diff

Review of attachment 8790117 [details] [diff] [review]:
-----------------------------------------------------------------

The code change looks good, but the patch also contains whitespace changes to unrelated code. (You can see them if you click "Splinter Review" beside the patch.)

It looks like your editor might be configured to remove trailing whitespace from all lines in the file, including lines that weren't edited.

We don't like to include unrelated whitespace changes in a patch, because it confuses people using "hg annotate" (which shows what was the last change made to a given line).

Could you create a version of the patch that doesn't include the unrelated whitespace changes? Thanks!
Attachment #8790117 - Flags: review?(botond) → feedback+
Flags: needinfo?(botond)
Attachment #8790400 - Flags: review?(botond)
Hello,

So I clicked off the remove trailing white spaces option in my editor... is this good?
Comment on attachment 8790400 [details] [diff] [review]
bug1301126_Restrict_scroll_to_chrome.diff

Review of attachment 8790400 [details] [diff] [review]:
-----------------------------------------------------------------

Much better, thanks! Just one tiny issue remaining:

::: dom/html/nsGenericHTMLElement.cpp
@@ +1675,5 @@
>  
>  /* static */ bool
>  nsGenericHTMLElement::IsScrollGrabAllowed(JSContext*, JSObject*)
>  {
>    // Only allow scroll grabbing in chrome and certified apps.

Please adjust this comment as before
Attachment #8790400 - Flags: review?(botond) → feedback+
Attachment #8790117 - Attachment is obsolete: true
Attachment #8790400 - Attachment is obsolete: true
Attachment #8790405 - Flags: review?(botond)
Comment on attachment 8790405 [details] [diff] [review]
bug1301126_Restrict_scroll_to_chrome.diff

Review of attachment 8790405 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!
Attachment #8790405 - Flags: review?(botond) → review+
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4471e056627
Restrict scroll grabbing to only chrome callers. r=botond
I committed the patch on mozilla-inbound. It should get merged into mozilla-central within a day or so.
Oh cool! 

Can you please provide any resources on where to proceed to next? I have been trying to understand the code base... but it is so immense!  The documentation is outrageously large, like one link leads to hundreds of other and I get lost all the time. 

- Kevin
(In reply to kevin from comment #18)
> Can you please provide any resources on where to proceed to next? I have
> been trying to understand the code base... but it is so immense!  The
> documentation is outrageously large, like one link leads to hundreds of
> other and I get lost all the time.

Definitely, Firefox is a huge project, and no one person can hope to get a handle on the entire codebase. I've been working on Firefox for over three years, and still, besides the specific areas of the code that I work on (maybe a few dozen files), most of the rest of the codebase is still very unfamiliar to me.

I would suggest continuing to work on mentored bugs, which are progressively more challenging. If you have in mind a particular area you're interested in (e.g. in terms of functionality), you're welcome to choose bugs in that area. Otherwise, you can just try working on bugs in different areas until something strikes your interest.

A good place for finding more mentored bugs to work on is this site: http://www.joshmatthews.net/bugsahoy/

Of the bugs that I'm mentoring, there are two that are currently not taken: bug 1180865, and bug 1204502. They're both probably a bit too involved to make a good second bug, although you could try bug 1204502 if you're up for a challenge (or circle back to it after fixing a couple of other easier ones).
https://hg.mozilla.org/mozilla-central/rev/e4471e056627
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.