Fixing a bug on opening source environment is a communication

The bug I chose was a brackets bug. “`allowJavascript` preference not honored on refresh”

Issue #606

Introduction of this bug

For the previous bug fixing, I only need read the requirement, ask where to start, fix it, and send a pull request. However, for this bug, I send a pull request, but they don’t accept it because they want to fix this bug on brackets side. After I fix a bug on brackets side and send a pull request, they want to change a little bit codes. In opening source environment, there should be a lot of discussions about determining what the best way to fix a bug.

Here are the details how I fix this bug

Fixing a bug on thimble side

When the toggle is clicked, it does 3 things.

1.Set the UI
2.analytics.event()
3.disable/ enable JavaScript

However, it only sets the UI when it initials. We can call the enable/disable function when it initials.

Pull request

This change fixes this bug very well. However, they don’t accept it because brackets and thimble are two products. The bug is on bracket side, so we should not fix it on thimble side.

 

Look for similar bug

First of all, I play with this program. Link of this product

捕获.PNG

There are two toggles on the allow javaScript toggle. I disable the wrap long line and refresh the page. Bracket doesn’t wrap any long line. It means the bug of wrap long lines has been fixed. I search on GitHub for wrap long lines and color theme, but I can’t get any useful pull request or issues. It means I should look at how the system disables javaScript because this is a specific bug only for ‘Allow JavaScript’.

Using preference system instead local variable

First of all, I should locate to the codes which define how to disable javaScript. I can disable on thimble with bramble.disableJavaScript(). I can look at this function and see what it does.

git grep disableJavaScript

I locate this function and find another keyword “BRAMBLE_ENABLE_SCRIPTS”

git grep BRAMBLE_ENABLE_SCRIPTS

it calls this function “HTMLRewriter.enableScripts();”

1. Search “HTMLRewriter” on this file

var HTMLRewriter = brackets.getModule(filesystem/impls/filer/lib/HTMLRewriter);

  2. Open HTMLRewriter file, and search “enableScripts()”

exports.enableScripts = function() {
jsEnabled = true;
};

This function only does one thing – change a value of this variable.

 3. Search and read the documentation of this variable.

/**
– * This variable controls whether or not we want scripts to be run in the preview window or not
– * We do this by altering the mime type from text/javascript to text/x-scripts-disabled below.
– */
– var jsEnabled = true;

It sounds this is what I am looking for.

 4. Change it to false and test it.

Editor disables javaScript and the alert doesn’t show – This is what I expect.

5. Change it to true and test it

The editor enables javaScript and the alert shows up – This is also what I expect.

6. Set this variable always equal to preference and send a pull request

Pull request

They don’t accept it at first because they want to keep a local variable.

7. Keep the local variable and get merged

 

What I can improve for next fix

  • The title of my pull request is “Fix issue606”. I should change it like “Fix issue606 – `allowJavascript` preference not honored on refresh”
  • I don’t why they want to keep the local variable, and I should ask why!!!

 

 

Advertisements

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s